[Feature] percentfield and showperc with top and rare commands. - #5642
[Feature] percentfield and showperc with top and rare commands.#5642AjimelecGonzalez wants to merge 1 commit into
percentfield and showperc with top and rare commands.#5642Conversation
PR Reviewer Guide 🔍(Review updated until commit ca50dab)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to ca50dab Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 840ef88
Suggestions up to commit 19f05c0
Suggestions up to commit 840ef88
Suggestions up to commit 3bea92e
Suggestions up to commit 84c6094
|
54ab043 to
ca7e450
Compare
|
Persistent review updated to latest commit ca7e450 |
ca7e450 to
dae1ecd
Compare
|
Persistent review updated to latest commit dae1ecd |
dae1ecd to
0b5b6ed
Compare
|
Persistent review updated to latest commit 0b5b6ed |
0b5b6ed to
0039627
Compare
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 0039627.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
|
Persistent review updated to latest commit 0039627 |
0039627 to
b576e5a
Compare
|
Persistent review updated to latest commit b576e5a |
b576e5a to
be841df
Compare
|
Persistent review updated to latest commit be841df |
be841df to
84c6094
Compare
|
Persistent review updated to latest commit 84c6094 |
84c6094 to
3bea92e
Compare
|
Persistent review updated to latest commit 3bea92e |
showperc with top and rare commands.showperc and percentfield with top and rare commands.
showperc and percentfield with top and rare commands.percentfield and showperc with top and rare commands.
3bea92e to
840ef88
Compare
|
Persistent review updated to latest commit 840ef88 |
|
Persistent review updated to latest commit 19f05c0 |
19f05c0 to
840ef88
Compare
|
Persistent review updated to latest commit 840ef88 |
* Clean docs * Fix CalcitePPLRareTopNTest showperc tests * Calculate percentages before filtering * Added percentfield and changed decimal places from 2 to 6 * Update test errors, SUM to CHECKED_LONG_SUM Signed-off-by: Ajimelec Gonzalez <ajimelec@amazon.com>
840ef88 to
ca50dab
Compare
|
Persistent review updated to latest commit ca50dab |
| throw new IllegalArgumentException( | ||
| "Field `" | ||
| + percentFieldName | ||
| + "` already exists in the output. Consider renaming with percentfield='xyz'"); |
There was a problem hiding this comment.
suggestion: Would it be better user experience if we provided a new percent field automatically?
e.g. generate percent0, percent1, etc. Joins already do this with join IDs (id -> id0, etc).
There was a problem hiding this comment.
Sure, will remove the check.
| List.of(), | ||
| partitionKeys, | ||
| List.of(), | ||
| WindowFrame.rowsUnbounded()); |
There was a problem hiding this comment.
issue: This has a correctness issue for high-cardinality fields because the total is based on the returned buckets.
The current impl works well for low-cardinality fields, but for high cardinality we get a truncated bucket set. I'm worried about long-tail types of top-N cases (e.g. "top 10 endpoints" from a service with many assets). rare would be much more affected in that case.
In principle we should be using the sum_other_doc_count value that's returned by aggregations if it's available: https://docs.opensearch.org/latest/aggregations/bucket/terms/#example-response. But fixing may be complex depending on the exact conditions where that value is present, I don't know them off the top but it may only apply to term aggregations and not composite aggs, and it means percent aggregations need to be finalized at the coordinator. If it's not available then the "right" way is to submit a whole separate _count request but that'll complicate query flow.
If it's a lot to patch, I think we can leave this behavior, but should document the limitation. You can manually work around it by using a subquery or something like eventstats to work with stats count() computed ahead of time. (Alternatively, if such a method exists, we could rewrite the query plan to be equivalent to it.)
There was a problem hiding this comment.
Will look into it and document, I believe that the using the sum_other_doc_count value would be the best choice here but will have to deep dive. For now I will add the limitations to the md file for top and rare commands.
Description
Add
percentfieldandshowpercoptions to thetopandrarePPL commands. Whenshowperc=trueis specified, a percent column is added to the output showing each value's percentage of the total count within its partition (group-by clause). Thepercentfieldoption allows customizing the name of the percentage column (default: percent).The percentage is computed as
ROUND(100.0 * count / SUM(count) OVER (PARTITION BY group_fields), 6)and is rounded to 6 decimal places. When used with a by clause, percentages are calculated within each partition (summing to 100% per group).Syntax
source=index | top [percentfield='<name>'] showperc=true field_name [by group_field]source=index | rare [percentfield='<name>'] showperc=true field_name [by group_field]TopExamplesource=otellogs | top showperc=true severityTextReturns:
RareExamplesource=otellogs | rare showperc=true severityTextReturns:
Custom Percentage Field Name Example
source=otellogs | top percentfield='pct' showperc=true severityTextReturns:
Related Issues
Resolves #5530
Check List
--signoffor-s.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.