Add summarization, limit, and stats-skip to _cat APIs - #22502
Conversation
github-actions
Bot
added
the
enhancement
PR Reviewer Guide 🔍(Review updated until commit e38cfd0)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to e38cfd0 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 4d1d7d7
Suggestions up to commit 7daf310
Suggestions up to commit bd043ec
Suggestions up to commit fc49a34
Suggestions up to commit 680b92c
|
|
❌ Gradle check result for 680b92c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
To fix this use the AccessController replacement within this repo: https://github.com/opensearch-project/OpenSearch/blob/main/libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java Edit: That's not the cause of failure. The cause was further below |
|
Persistent review updated to latest commit fc49a34 |
|
❌ Gradle check result for fc49a34: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Persistent review updated to latest commit bd043ec |
|
❌ Gradle check result for bd043ec: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Persistent review updated to latest commit 7daf310 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22502 +/- ##
============================================
- Coverage 71.42% 71.34% -0.08%
+ Complexity 76786 76745 -41
============================================
Files 6148 6148
Lines 357980 358021 +41
Branches 52177 52192 +15
============================================
- Hits 255689 255433 -256
- Misses 81937 82232 +295
- Partials 20354 20356 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Address the remaining review comments on PR opensearch-project#22502: - Version gate for the new indicesStatsRequired field in CatShardsRequest moves from V_3_7_0 to V_3_8_0. The 3.8.0 branch has been cut from main, so this change lands in 3.9.0-development; gating on V_3_8_0 keeps the intent (feature only serialized between peers that have it) without fabricating a V_3_9_0 constant that doesn't yet exist in Version.java. Adds mixed-version wire-compatibility tests covering the round-trip on Version.CURRENT and the safe default (indicesStatsRequired=true) when the negotiated peer version is before the gate. - RestTable.buildResponse now reads h= via request.params().get("h") instead of request.param("h"). The buildDisplayHeaders call later in the pipeline still calls request.param("h") and marks it consumed for unrecognized-parameter bookkeeping; using the non-consuming raw map here keeps that ordering unchanged. - TableSummarizer.copyHeaderAttrString now sanitizes ';' and ':' inside attribute values (replacing them with spaces) instead of silently dropping the whole attribute. This prevents a future header with a colon in its 'desc' from losing metadata during the summarize round trip. Includes a unit test that exercises the sanitizer directly with a manually-constructed attr map (Table.addCell's parser can't itself represent a value containing ':', which is precisely the scenario the sanitizer defends against). - Aggregator now tracks countNonNull separately from countAll and countContributing. count(field) returns countNonNull, matching SQL COUNT(field) semantics — a group with three rows one of which is null now reports count(field)=2. Updated the two existing avg/null tests to assert the new semantics. - TableSummarizer.summarize validates that every column referenced in h= (both bare group-by tokens and func(field) arguments) resolves to an existing table column, either directly or via aliasMap. Unknown tokens raise IllegalArgumentException with a message naming the offending token so users no longer get silent empty output from a typo. Tests cover the error paths and confirm alias references continue to work. Test count after this change (cat suites): CatShardsRequestTests=6, TableSummarizerTests=33, RestTableTests=28, all passing. Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
|
Persistent review updated to latest commit 4d1d7d7 |
Add grouped + aggregated views to _cat APIs, expressed entirely through the h= parameter: a bare column token (e.g. index) is a GROUP BY key and a func(field) token (sum, count, avg, min, max) is an aggregate over the grouped rows, mirroring SELECT index, sum(docs) ... GROUP BY index. When h= contains no aggregation function the table is returned unchanged, so ordinary listings behave exactly as before. Because the group key is always a bare h= token, it is always rendered. Aggregation preserves numeric source types (Long/Integer/Short/Byte, ByteSizeValue, SizeValue, TimeValue) so downstream bytes=/time=/size= rendering keeps working, and avg divides by the count of non-null values so null cells do not drag the average toward zero. Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
Add a 'limit' query parameter that caps the number of rows in a _cat response. Applied at the RestTable level after sort, so ?s=field:desc&limit=N yields the top-N rows by the sort key. The limit applies uniformly to summarized and unsummarized output: - Without aggregation: caps the response to N rows - With aggregation: caps to N groups, with sort applied to the aggregated result so aggregation + sort + limit yields the top-N groups by sort key Example: GET _cat/segments?h=index,sum(docs.count)&s=sum(docs.count):desc&limit=5 Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
The _cat/shards transport always issued an IndicesStats broadcast to every data node, even when the response did not need a single stats-derived column. For high-shard clusters this is a substantial fan-out cost that contributed nothing to the response. Detect at the REST layer whether any column in h= or s= requires per-shard stats. When all referenced columns are derivable from cluster state (index, shard, prirep, state, node/ip/id, unassigned.*, recoverysource.type, and their aliases), set a new indicesStatsRequired=false flag on the CatShardsRequest. The transport action honors the flag by returning an empty IndicesStatsResponse and short-circuiting the broadcast. The fast path is intentionally conservative: - h= must be explicitly set (default headers include docs/store) - every h= and s= token must match a known routing-only column - wildcards force the slow path (we don't expand them here) - unknown tokens force the slow path Backwards compatibility for the new request field is gated on Version.V_3_7_0 with the default (true) preserving today's behavior on mixed-version clusters. Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
forbiddenApisTest failed because TableSummarizerTests reached the private RestTable#renderValue via Class#getDeclaredMethod + AccessibleObject#setAccessible, which the check bans. Make RestTable#renderValue package-private (the test is in the same package) and call it directly, removing the reflection and the now unused RestRequest import. Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
Address review feedback on the _cat summarization/limit change: - TableSummarizer.formatValue now converts aggregated doubles to long via a safeLong helper that rounds half-up and clamps to Long.MIN/MAX_VALUE instead of truncating and silently overflowing. This prevents a sum of large ByteSizeValues from wrapping to a negative value (which would throw from the ByteSizeValue constructor). - AGG_FUNC_PATTERN is compiled CASE_INSENSITIVE so SUM(docs)/Count(shard) are recognized consistently by hasAggregation and summarize. - RestShardsAction stats-skip sort parsing trims whitespace and strips the :asc/:desc suffix case-insensitively, so index:DESC or " shard:Asc" still resolve to routing-only columns and take the fast path. Adds tests for case-insensitive aggregation, safeLong rounding/clamping, and mixed-case/whitespace sort suffixes, and updates the TimeValue avg assertion to the corrected round-half-up result. Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
Address the remaining review comments on PR opensearch-project#22502: - Version gate for the new indicesStatsRequired field in CatShardsRequest moves from V_3_7_0 to V_3_8_0. The 3.8.0 branch has been cut from main, so this change lands in 3.9.0-development; gating on V_3_8_0 keeps the intent (feature only serialized between peers that have it) without fabricating a V_3_9_0 constant that doesn't yet exist in Version.java. Adds mixed-version wire-compatibility tests covering the round-trip on Version.CURRENT and the safe default (indicesStatsRequired=true) when the negotiated peer version is before the gate. - RestTable.buildResponse now reads h= via request.params().get("h") instead of request.param("h"). The buildDisplayHeaders call later in the pipeline still calls request.param("h") and marks it consumed for unrecognized-parameter bookkeeping; using the non-consuming raw map here keeps that ordering unchanged. - TableSummarizer.copyHeaderAttrString now sanitizes ';' and ':' inside attribute values (replacing them with spaces) instead of silently dropping the whole attribute. This prevents a future header with a colon in its 'desc' from losing metadata during the summarize round trip. Includes a unit test that exercises the sanitizer directly with a manually-constructed attr map (Table.addCell's parser can't itself represent a value containing ':', which is precisely the scenario the sanitizer defends against). - Aggregator now tracks countNonNull separately from countAll and countContributing. count(field) returns countNonNull, matching SQL COUNT(field) semantics — a group with three rows one of which is null now reports count(field)=2. Updated the two existing avg/null tests to assert the new semantics. - TableSummarizer.summarize validates that every column referenced in h= (both bare group-by tokens and func(field) arguments) resolves to an existing table column, either directly or via aliasMap. Unknown tokens raise IllegalArgumentException with a message naming the offending token so users no longer get silent empty output from a typo. Tests cover the error paths and confirm alias references continue to work. Test count after this change (cat suites): CatShardsRequestTests=6, TableSummarizerTests=33, RestTableTests=28, all passing. Signed-off-by: Kunal Khatua <kkhatua@amazon.com>
|
Persistent review updated to latest commit e38cfd0 |
|
❕ Gradle check result for e38cfd0: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
2 participants
Description
Adds grouped + aggregated views, a limit parameter, and a stats fan-out optimization to the _cat APIs. Grouping and aggregation are expressed entirely through the existing
h=parameter, so a bare column token (e.g.action) is aGROUP BYkey and afunc(field)token (sum,count,avg,min,max) is an aggregate over the grouped rows. This mirrors the SQL mental modelSince grouping is inferred from
h=h=contains no aggregation function, the table is returned unchanged — ordinary_catlistings behave exactly as before.h=token, so it is always rendered (no invisible grouping keys or silently dropped columns).Aggregation preserves numeric source types (
Long/Integer/Short/Byte,ByteSizeValue,SizeValue,TimeValue) so downstreambytes=/time=/size=rendering keeps working on aggregated values.Also,
avg()divides by the count of non-null values sonullcells don’t drag the average toward zero.This feature is centralized in
RestTable.buildResponse, so every_catendpoint that renders throughRestTableinherits it (shards, tasks, segments, indices, nodes, aliases, recovery, …).Examples
1.
_cat/shardsRoll shards up per index — count, total docs, total store (largest first):
Group by index + primary/replica:
GET _cat/shards?h=index,prirep,count(shard),avg(docs)&vlimitapplies after sort, so this yields the top-5 indices by document count:GET _cat/shards?h=index,sum(docs)&s=sum(docs):desc&limit=5&vPlain listings are unchanged (no aggregation function present):
GET _cat/shards?h=index,shard,prirep,state,node&vThe perf commit additionally skips the cluster-wide IndicesStats broadcast entirely when every requested column is derivable from cluster state (e.g. the plain listing above), which is a meaningful savings on high-shard clusters.
2.
_cat/tasksAggregation works identically for tasks. The tables below were produced from a real 510-task _tasks snapshot:
Tasks per action, with running-time stats (a TimeValue, so units are preserved), most frequent first:
Group by task type:
Top nodes by number of running tasks:
GET _cat/tasks?h=node,count(task_id),max(running_time)&s=count(task_id):desc&limit=10&vRelated Issues
Resolves #22500
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.