Adds metadata to rewritten aggregations #28185
Conversation
| @@ -101,7 +101,7 @@ public final AggregationBuilder rewrite(QueryRewriteContext context) throws IOEx | |||
| if (rewritten == this) { | |||
| return rewritten; | |||
| } | |||
| if (getMetaData() != null && rewritten.getMetaData() == null) { | |||
| if (getMetaData() != null && (rewritten.getMetaData() == null || rewritten.getMetaData().isEmpty())) { | |||
There was a problem hiding this comment.
Shouldn't the agg be responsible for copying the metadata when it's rewritten ? This is a bit confusing now since we don't know if the rewritten metadata is empty on purpose or not. We could just copy the metadata explicitly in Filter(s)Agg...#doRewrite to avoid ambiguity ?
There was a problem hiding this comment.
The way I have been thinking about it is that this superclass manages the metadata in every other respect (toXContent, setters, getters) so it seem natural to me that it would handle the metadata while rewriting the agg? This way the way that metadata is handled should be consistent across all aggregations?
There was a problem hiding this comment.
Actually it is a bit confusing because AbractAggregationBuilder handles some of the metadata stuff too, but since most if not all aggregations extend AbstractAggregationBuilder I think it is still nice to have consistent logic for metadata across all the aggs. Maybe separately we should look at whether we still need AggregationBuilder and AbstractAggregationBuilder or if we could merge them to make things a bit simpler?
There was a problem hiding this comment.
Maybe separately we should look at whether we still need AggregationBuilder and AbstractAggregationBuilder or if we could merge them to make things a bit simpler?
agreed, it will also simplify the fix for #27782 which requires a new clone function.
Previous to this change, if any filters in the filters aggregation were rewritten, the rewritten version of the FiltersAggregationBuilder would not contain the metadata form the original. This is because `AbstractAggregationBuilder.getMetadata()` returns an empty map when not metadata is set. Closes #28170
|
jenkins test this please |
| @@ -101,7 +101,7 @@ public final AggregationBuilder rewrite(QueryRewriteContext context) throws IOEx | |||
| if (rewritten == this) { | |||
| return rewritten; | |||
| } | |||
| if (getMetaData() != null && rewritten.getMetaData() == null) { | |||
| if (getMetaData() != null && (rewritten.getMetaData() == null || rewritten.getMetaData().isEmpty())) { | |||
| rewritten.setMetaData(getMetaData()); | |||
| } | |||
There was a problem hiding this comment.
what prevents us from always blindly setting the metadata? Rewriting should never modify it?
There was a problem hiding this comment.
good point. I'll change it to do this since it simplifies things
* Adds metadata to rewritten aggregations Previous to this change, if any filters in the filters aggregation were rewritten, the rewritten version of the FiltersAggregationBuilder would not contain the metadata form the original. This is because `AbstractAggregationBuilder.getMetadata()` returns an empty map when not metadata is set. Closes #28170 * Always set metadata when rewritten
* master: (21 commits) [GEO] Add WKT Support to GeoBoundingBoxQueryBuilder Painless: Add whitelist extensions (elastic#28161) Fix daitch_mokotoff phonetic filter to use the dedicated Lucene filter (elastic#28225) Avoid doing redundant work when checking for self references. (elastic#26927) Fix casts in HotThreads. (elastic#27578) Ignore the `-snapshot` suffix when comparing the Lucene version in the build and the docs. (elastic#27927) Allow update of `eager_global_ordinals` on `_parent`. (elastic#28014) Fix NPE on composite aggregation with sub-aggregations that need scores (elastic#28129) `MockTcpTransport` to connect asynchronously (elastic#28203) Fix synonym phrase query expansion for cross_fields parsing (elastic#28045) Introduce elasticsearch-core jar (elastic#28191) elastic#28218: Update the Lucene version for 6.2.0 after backport upgrade to lucene 7.2.1 (elastic#28218) [Docs] Fix an error in painless-types.asciidoc (elastic#28221) Adds metadata to rewritten aggregations (elastic#28185) Update version of TaskInfo header serialization after backport TEST: Tightens file-based condition in peer-recovery Correct backport replica rollback to 6.2 (elastic#28181) Backport replica rollback to 6.2 (elastic#28181) Rename deleteLocalTranslog to createNewTranslog ...
Previous to this change, if any filters in the filters aggregation were rewritten, the rewritten version of the FiltersAggregationBuilder would not contain the metadata form the original. This is because
AbstractAggregationBuilder.getMetadata()returns an empty map when not metadata is set.Closes #28170