Fix a bug with flattened fields in terms aggregations #87392
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
not-napoleon added >bug :Analytics/Aggregations 
| Hi @not-napoleon, I've created a changelog YAML for you. |
…on/elasticsearch into flattened-field-terms-agg-bug
👍 |
nik9000 reviewed Jun 6, 2022
| public class TermsAggregatorFactory extends ValuesSourceAggregatorFactory { | ||
| static Boolean REMAP_GLOBAL_ORDS, COLLECT_SEGMENT_ORDS; | ||
| | ||
| protected static final Logger logger = LogManager.getLogger(TermsAggregatorFactory.class); |
There was a problem hiding this comment.
I'd go with private just for a bit of paranoia.
For what it's worth LogManger.getLogger() is very very fast. I still use the getLogger(class) mostly though.
| } | ||
| | ||
| // TODO: [Zach] we might want refactor and remove ExecutionMode#create(), moving that logic outside the enum | ||
| logger.info("Creating bytes terms aggregator with execution mode [" + execution + "]"); |
There was a problem hiding this comment.
These'd probably be fine to keep with debug if you like. Though you should do the [{}]` substitution thing.
| @Override | ||
| public boolean supportsGlobalOrdinalsMapping() { | ||
| return valuesSource.supportsGlobalOrdinalsMapping(); | ||
| } |
elasticmachine added the Team:Analytics 
| Pinging @elastic/es-analytics-geo (Team:Analytics) |
| Pinging @elastic/clients-team (Team:Clients) |
nik9000 approved these changes Jun 7, 2022
not-napoleon added auto-backport This was referenced Jun 8, 2022
not-napoleon added a commit to not-napoleon/elasticsearch that referenced this pull request Jun 8, 2022
The root cause here was that missing did not correctly delegate `supportsGlobalOrdinalsMappnig` to the wrapped values source, instead falling back to the default. I've added the delegation, and made the base method abstract so this doesn't happen again.
not-napoleon added a commit to not-napoleon/elasticsearch that referenced this pull request Jun 8, 2022
The root cause here was that missing did not correctly delegate `supportsGlobalOrdinalsMappnig` to the wrapped values source, instead falling back to the default. I've added the delegation, and made the base method abstract so this doesn't happen again.
not-napoleon added a commit that referenced this pull request Jun 8, 2022
…87507) The root cause here was that missing did not correctly delegate `supportsGlobalOrdinalsMappnig` to the wrapped values source, instead falling back to the default. I've added the delegation, and made the base method abstract so this doesn't happen again.
not-napoleon added a commit that referenced this pull request Jun 8, 2022
…#87506) * Fix a bug with flattened fields in terms aggregations (#87392) The root cause here was that missing did not correctly delegate `supportsGlobalOrdinalsMappnig` to the wrapped values source, instead falling back to the default. I've added the delegation, and made the base method abstract so this doesn't happen again. * BWC version dance, part one * Fix flattened object format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 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.
The bug occurs when terms incorrectly chooses the
LowCardinalityoptimization with a flattened field. This can only happen ifsupportsGlobalOrdinalsMappingis true, which it usually is not for flattened fields. But, when we wrap theValuesSourceto add in the missing value, we weren't delegating that method, which led to this bug.Draft because I still need to do some more testing, and probably
supportsGlobalOrdinalsMappingshould be abstract so this doesn't happen anywhere else.