-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[Star Tree] Add date dimension support for range aggregation validation #20460
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?
[Star Tree] Add date dimension support for range aggregation validation #20460
Conversation
4cb3dbd to
e7de8d2
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds support for date type dimensions in range aggregation validation within the Star Tree query context. The validation logic is broadened to accept both numeric and date fields, with corresponding test case updates demonstrating the new capability. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cbd689b to
3d2a5c8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20460 +/- ##
============================================
- Coverage 73.24% 73.24% -0.01%
- Complexity 71961 71968 +7
============================================
Files 5795 5798 +3
Lines 329185 329333 +148
Branches 47403 47421 +18
============================================
+ Hits 241125 241222 +97
- Misses 68734 68811 +77
+ Partials 19326 19300 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This change extends the star-tree range aggregation validation to support DateDimension in addition to NumericDimension. Previously, range aggregations with star-tree optimization only worked with numeric fields. Now date fields can also leverage star-tree indexes for range aggregations. Signed-off-by: Anand Kumar Shaw <anandkrshawheritage@gmail.com>
3d2a5c8 to
e6110c3
Compare
|
❌ Gradle check result for e6110c3: 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? |
sandeshkr419
left a comment
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.
Thanks @anandheritage for working on this.
Can you also add a test in RangeAggregatorTests to validate the results.
It has been sometime I had worked on the codebase, but I strongly suspect we will need to do some validation on the ranges like I did for in range queries on date field: #17443
This is because how we store date fields as aggregated intervals and not raw date values.
Feel free to extract out common utilities as required.
Summary
DateDimensionin addition toNumericDimensionChanges
StarTreeQueryContext.java: UpdatedvalidateRangeAggregationSupport()to acceptDateDimensionSearchServiceStarTreeTests.java: Updated test Case 6 to verify date field range aggregations use star-treeCHANGELOG.md: Added entry for this featureTest plan
SearchServiceStarTreeTests.java./gradlew :server:test --tests "org.opensearch.search.aggregations.startree.*")Related Issues
Resolves the TODO in
StarTreeQueryContext.java: "Add support for date type ranges"