-
Notifications
You must be signed in to change notification settings - Fork 176
Scaffolding: top/rare useOther and percentage arguments #4335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Scaffolding: top/rare useOther and percentage arguments #4335
Conversation
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Swiddis , thanks for the change. Just left some comments. Also, I couldn't find any rst doc change in this PR, so will it be also included in the 2 upcoming PR you mentioned in the PR description?
core/src/main/java/org/opensearch/sql/ast/tree/args/RareTopNArguments.java
Show resolved
Hide resolved
ppl/src/test/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizerTest.java
Outdated
Show resolved
Hide resolved
Doc changes will come when the features are actually supported -- they'll currently just raise errors so no point documenting (they'd fail doctest anyway) |
Signed-off-by: Simeon Widdis <[email protected]>
- id: spotless-check | ||
name: Spotless Post-format Check | ||
entry: bash -c './gradlew spotlessCheck' | ||
entry: bash -c './gradlew spotlessApply spotlessCheck && git add -u' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small build change, makes precommit faster (check always runs after apply)
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Description
PR 1/3: this PR adds some parsing logic and does a bit of plumbing related to upcoming arguments. This doesn't include the actual implementation. useOther and showPerc will be the other two PRs.
Depending on how complex those are, I might also be able to hit #4278 and #4282 in the same go, since composite buckets of terms support sorting by count. I'd also want to switch to the rare_terms aggregator for
rare
, per the warning on the linked doc.Exactly how to get a total for other/percentage is still somewhat open. I thought earlier that I could subtract from a cheap
_count
call, but I didn't notice at the time that you could also supply filters before the inputs which invalidates that method. Going to poke at the DSL a bit more & see what we end up with (worst case we end up going by all returned buckets, which hits issues if there's too many buckets).Related Issues
N/A
Check List
--signoff
or-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.