Continue realizing sorting by aggregations #52298
Merged
Conversation
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
This drops more of the `instanceof`s from `AggregationPath`. There are still a couple in `AggregationPath`. And I ended up moving two into `BucketsAggregator`, but I think this is still an improvement!
nik9000
commented
Feb 13, 2020
|
|
||
| @Override | ||
| public Aggregator resolveSortPath(AggregationPath.PathElement next, Iterator<AggregationPath.PathElement> path) { | ||
| if (this instanceof SingleBucketAggregator) { |
Member
Author
There was a problem hiding this comment.
I'm not super thrilled about this, but it is how it used to work and I wanted to keep this change fairly contained.
| * that sort path itself can't contain a different {@code terms} | ||
| * aggregation. | ||
| */ | ||
| public final Aggregator resolveSortPathOnValidAgg(AggregationPath.PathElement next, Iterator<AggregationPath.PathElement> path) { |
Member
Author
There was a problem hiding this comment.
I'm not super comfortable with this name, but I think it'll fall away in a followup change. When I dig into that last instanceof in resolveTopmostAggregator.
Collaborator
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
polyfractal
approved these changes
Feb 21, 2020
Contributor
polyfractal
left a comment
polyfractal
left a comment
There was a problem hiding this comment.
Apologies for the delay. ++, this is indeed cleaner than a big monolithic blob of checks :)
|
|
||
| @Override | ||
| public Aggregator resolveSortPath(AggregationPath.PathElement next, Iterator<AggregationPath.PathElement> path) { | ||
| if (this instanceof SingleBucketAggregator) { |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Feb 21, 2020
This drops more of the `instanceof`s from `AggregationPath`. There are still a couple in `AggregationPath`. And I ended up moving two into `BucketsAggregator`, but I think this is still an improvement!
Member
Author
|
Thanks @polyfractal ! |
nik9000
added a commit
that referenced
this pull request
Feb 21, 2020
This reverts commit 5659e72. It causes some test failures that didn't come up in the PR tests.
Member
Author
|
I've reverted this because it caused a test failure that we didn't catch in the PR tests. I'll dig on Monday. |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Feb 23, 2020
This reverts commit b3afbe7 which reverted #53398 because we encountered a test failure after merging that hadn't shown up before. Sneaky!
nik9000
added a commit
that referenced
this pull request
Feb 23, 2020
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Feb 23, 2020
…astic#52682) This reverts commit b3afbe7 which reverted #53398 because we encountered a test failure after merging that hadn't shown up before. Sneaky!
nik9000
added a commit
that referenced
this pull request
Feb 23, 2020
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 drops more of the
instanceofs fromAggregationPath. There arestill a couple in
AggregationPath. And I ended up moving two intoBucketsAggregator, but I think this is still an improvement!