Modernize cardinality agg tests#90114
Merged
not-napoleon merged 7 commits intoelastic:mainfrom Sep 21, 2022
Merged
Conversation
Collaborator
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
not-napoleon
commented
Sep 15, 2022
| assertTrue(AggregationInspectionHelper.hasValue(cardinality)); | ||
|
|
||
| // Test that an aggregation not using a script does get cached | ||
| assertTrue(context.isCacheable()); |
Member
Author
There was a problem hiding this comment.
This assert is the only thing this test adds over the refactored test above. I don't know where we should be testing that SearchExecutionContext is setting this correctly, but I don't think this is the right place. And having an aggregation context reference at this point is not good. Let's discuss if this test adds enough value that we should re-create it somewhere closer to the code actually being tested.
Member
There was a problem hiding this comment.
👍
I wonder if we should assert that all aggs are cacheable in test-case unless we have some kind of flag.
Member
Author
There was a problem hiding this comment.
That seems sensible. I'll try that and see if anything breaks.
Member
Author
There was a problem hiding this comment.
So exactly two tests broke, both of which are sorting by a script so it makes sense they shouldn't be cached. With #90149 merged, I am able to communicate that information to the right place in the test framework, with minimal intrusion on the rest of the code. So I think we should be all set here.
Member
|
Nice!
…On Tue, Sep 20, 2022, 3:43 PM Mark Tozzi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
server/src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
<#90114 (comment)>
:
> - final CardinalityAggregationBuilder aggregationBuilder = new CardinalityAggregationBuilder("cardinality").field("number");
-
- final AggregationContext context = createAggregationContext(indexSearcher, null, fieldType);
- final CardinalityAggregator aggregator = createAggregator(aggregationBuilder, context);
- aggregator.preCollection();
- indexSearcher.search(new MatchAllDocsQuery(), aggregator.asCollector());
- aggregator.postCollection();
-
- final InternalCardinality cardinality = (InternalCardinality) aggregator.buildAggregation(0L);
-
- assertEquals(10.0, cardinality.getValue(), 0);
- assertEquals("cardinality", cardinality.getName());
- assertTrue(AggregationInspectionHelper.hasValue(cardinality));
-
- // Test that an aggregation not using a script does get cached
- assertTrue(context.isCacheable());
So exactly two tests broke, both of which are sorting by a script so it
makes sense they shouldn't be cached. With #90149
<#90149> merged, I am able
to communicate that information to the right place in the test framework,
with minimal intrusion on the rest of the code. So I think we should be all
set here.
—
Reply to this email directly, view it on GitHub
<#90114 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUXIUI6C35QOAUBBI2NEDV7IHWRANCNFSM6AAAAAAQNYKTSQ>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
nik9000
approved these changes
Sep 20, 2022
Member
Author
|
@elasticmachine update branch |
weizijun
added a commit
to weizijun/elasticsearch
that referenced
this pull request
Sep 22, 2022
* main: (186 commits) [DOCS] Add 8.5 release notes and fix links (elastic#90201) Mute DownsampleActionSingleNodeTests.testCannotRollupWhileOtherRollupInProgress (elastic#90198) Bump to version 8.6.0 Increase the minimum size of the management pool to 2 (elastic#90193) Speed `getIntLE` from `BytesReference` (elastic#90147) Restrict nodes for testClusterPrimariesActive1 (elastic#90191) Fix bug with `BigIntArray` serialization (elastic#90142) Synthetic _source: test _source filtering (elastic#90138) Modernize cardinality agg tests (elastic#90114) Mute failing test (elastic#90186) Move assertion in ES85BloomFilterPostingsFormat to fix test (elastic#90150) Restrict nodes for testClusterPrimariesActive2 (elastic#90184) Batch index delete cluster state updates (elastic#90033) Register stable plugins in ActionModule (elastic#90067) Mute failing test (elastic#90180) [HealthAPI] Disk: Use _ for diagnosis id (elastic#90179) [HealtAPI] Disk: use shorter help URLs (elastic#90178) Fixing disk health indicator unit tests (elastic#90175) Enable the health node and the disk health indicator elastic#84811 (elastic#90085) Add missing Disk Indicator health api IDs (elastic#90174) ...
weizijun
added a commit
to weizijun/elasticsearch
that referenced
this pull request
Sep 22, 2022
* main: (121 commits) [DOCS] Add 8.5 release notes and fix links (elastic#90201) Mute DownsampleActionSingleNodeTests.testCannotRollupWhileOtherRollupInProgress (elastic#90198) Bump to version 8.6.0 Increase the minimum size of the management pool to 2 (elastic#90193) Speed `getIntLE` from `BytesReference` (elastic#90147) Restrict nodes for testClusterPrimariesActive1 (elastic#90191) Fix bug with `BigIntArray` serialization (elastic#90142) Synthetic _source: test _source filtering (elastic#90138) Modernize cardinality agg tests (elastic#90114) Mute failing test (elastic#90186) Move assertion in ES85BloomFilterPostingsFormat to fix test (elastic#90150) Restrict nodes for testClusterPrimariesActive2 (elastic#90184) Batch index delete cluster state updates (elastic#90033) Register stable plugins in ActionModule (elastic#90067) Mute failing test (elastic#90180) [HealthAPI] Disk: Use _ for diagnosis id (elastic#90179) [HealtAPI] Disk: use shorter help URLs (elastic#90178) Fixing disk health indicator unit tests (elastic#90175) Enable the health node and the disk health indicator elastic#84811 (elastic#90085) Add missing Disk Indicator health api IDs (elastic#90174) ...
34 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This converts one test in the Cardinality tests to use (a new multi index version of) test case, which will eventually enable better memory accounting testing. It also deletes a second test, which was only verifying that the aggregation context has the correct cache settings - that's not cardinality related, and testing it here is actually a PITA. We're trying to push all aggregation context handling into one path, so we can make sure it's accessed correctly with regards to closing.