GlobalAggregation with profile option enabled returns incorrect result #7114
Conversation
sohami
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #7114 +/- ##
=========================================
Coverage 70.65% 70.65%
+ Complexity 59453 59438 -15
=========================================
Files 4852 4852
Lines 285125 285127 +2
Branches 41104 41105 +1
=========================================
+ Hits 201441 201464 +23
+ Misses 67131 67124 -7
+ Partials 16553 16539 -14
... and 507 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -87,6 +87,7 @@ public void testWithStatsSubAggregator() throws Exception { | |||
| SearchResponse response = client().prepareSearch("idx") | |||
| .setQuery(QueryBuilders.termQuery("tag", "tag1")) | |||
| .addAggregation(global("global").subAggregation(stats("value_stats").field("value"))) | |||
| .setProfile(randomBoolean()) | |||
There was a problem hiding this comment.
The test should probably very that results are now correct (vs incorrect results before the fix).
There was a problem hiding this comment.
The test should probably very that results are now correct (vs incorrect results before the fix).
The results are asserted below in this test which was failing with profile=true before the fix.
There was a problem hiding this comment.
Using randomized testing for coverage is generally an anti-pattern. Probably better to explicitly test both cases of setProfile.
There was a problem hiding this comment.
@andrross I thought about it but the post mentions For instance the number of shards should not impact date_histogram aggregations, and the choice of the store type (niofs vs mmapfs) does not affect the results of a query. Such randomization helps improve confidence that we are not relying on implementation details of one component or specifics of some setup.
In this case since profiling should not change the results of query but returns extra information in the response I thought randomization should be fine
There was a problem hiding this comment.
My thinking is that at the aggregation layer, something like store type is completely abstracted because all interaction is done through something like a generic directory interface and the aggregation layer has no knowledge of the specific implementation. In that case randomization is fine because it's essentially just verifying nothing is leaking through the abstraction layers. In the profiling case there is now a specific if condition in the aggregation code, so both paths should be explicitly tested.
There was a problem hiding this comment.
@andrross Have updated the test to explicitly add for both the profile enabled/disabled case
| @@ -87,6 +87,7 @@ public void testWithStatsSubAggregator() throws Exception { | |||
| SearchResponse response = client().prepareSearch("idx") | |||
| .setQuery(QueryBuilders.termQuery("tag", "tag1")) | |||
| .addAggregation(global("global").subAggregation(stats("value_stats").field("value"))) | |||
| .setProfile(randomBoolean()) | |||
There was a problem hiding this comment.
Using randomized testing for coverage is generally an anti-pattern. Probably better to explicitly test both cases of setProfile.
Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
opensearch-project#7114) Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
Description
For the case when profile is enabled the check for GlobalAggregator operator will fail as it gets wrapped in the Profiling Aggregator. To fix this the check considers both the cases with profile enabled/disabled and verifies if the unwrapped aggregator instance is of Global type or not.
Issues Resolved
#7088
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.