Add a shallow copy method to aggregation builders #28430

jimczi · 2018-01-29T22:20:56Z

This change adds a shallow copy method for aggregation builders. This method returns a copy of the builder replacing the factoriesBuilder and metaData.
This method is used when the builder is rewritten (AggregationBuilder#rewrite) in order to make sure that we create a new instance of the parent builder when sub aggregations are rewritten.

Relates #27782

jimczi · 2018-01-29T22:21:13Z

@colings86 sorry ;), I asked for help because I thought that each builder would need a specific test for the added method but BaseAggregationTestCase can
test it simply so I decided to do it in a single pr. I'll add a specific test for #27782 in a follow up.

colings86

@jimczi I think it mostly looks good, my only comments are around the fields in the clone potentially being mutated and affecting the original which I think we need to protect against.

colings86 · 2018-01-30T12:15:55Z

server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java

@@ -52,6 +55,10 @@ protected AggregationBuilder(String name) {
        this.name = name;
    }

+    protected AggregationBuilder(AggregationBuilder clone) {
+        this.name = clone.name;


if this is supposed to produce and exact clone of the passed in builder then should we not set the factoriesBuilder here too?

colings86 · 2018-01-30T12:20:36Z

...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java

+    protected AbstractRangeBuilder(AbstractRangeBuilder<AB, R> clone) {
+        super(clone);
+        this.rangeFactory = clone.rangeFactory;
+        this.ranges = clone.ranges;


I think we might want to copy the list here rather than share it between the old and new copies? Otherwise if the builder is cloned and then addRange is called on the copy it will also change the original?

colings86 · 2018-01-30T12:21:12Z

...n/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java

+        this.distanceType = clone.distanceType;
+        this.unit = clone.unit;
+        this.keyed = clone.keyed;
+        this.ranges = clone.ranges;


I think we should copy the ranges list?

colings86 · 2018-01-30T12:21:27Z

.../main/java/org/elasticsearch/search/aggregations/bucket/range/IpRangeAggregationBuilder.java

@@ -222,6 +222,17 @@ public IpRangeAggregationBuilder(String name) {
        super(name, ValuesSourceType.BYTES, ValueType.IP);
    }

+    protected IpRangeAggregationBuilder(IpRangeAggregationBuilder clone) {
+        super(clone);
+        this.ranges = clone.ranges;


I think we should copy the ranges list?

colings86 · 2018-01-30T12:22:41Z

...elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java

@@ -129,6 +129,20 @@ public SignificantTermsAggregationBuilder(StreamInput in) throws IOException {
        significanceHeuristic = in.readNamedWriteable(SignificanceHeuristic.class);
    }

+    protected SignificantTermsAggregationBuilder(SignificantTermsAggregationBuilder clone) {
+        super(clone);
+        this.bucketCountThresholds = clone.bucketCountThresholds;


It seems that BucketCountThresholds is mutable so I think we need to clone it as well?

colings86 · 2018-01-30T12:23:42Z

.../elasticsearch/search/aggregations/bucket/significant/SignificantTextAggregationBuilder.java

@@ -123,6 +123,22 @@ public AggregationBuilder parse(String aggregationName, XContentParser parser)
        };
    }

+    protected SignificantTextAggregationBuilder(SignificantTextAggregationBuilder clone) {
+        super(clone);
+        this.bucketCountThresholds = clone.bucketCountThresholds;


It seems that BucketCountThresholds is mutable so I think we need to clone it as well?

colings86 · 2018-01-30T12:24:10Z

.../elasticsearch/search/aggregations/bucket/significant/SignificantTextAggregationBuilder.java

+        super(clone);
+        this.bucketCountThresholds = clone.bucketCountThresholds;
+        this.fieldName = clone.fieldName;
+        this.filterBuilder = clone.filterBuilder;


Is this filterBuilder potentially mutable too?

colings86 · 2018-01-30T12:24:24Z

...rc/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java

+        this.executionHint = clone.executionHint;
+        this.includeExclude = clone.includeExclude;
+        this.collectMode = clone.collectMode;
+        this.bucketCountThresholds = clone.bucketCountThresholds;


It seems that BucketCountThresholds is mutable so I think we need to clone it as well?

colings86 · 2018-01-30T12:26:07Z

...in/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregationBuilder.java

+        this.storedFieldsContext = clone.storedFieldsContext;
+        this.fieldDataFields = clone.fieldDataFields;
+        this.scriptFields = clone.scriptFields;
+        this.fetchSourceContext = clone.fetchSourceContext;


Are any of these object mutable?

jimczi · 2018-01-31T10:05:52Z

Thanks for reviewing @colings86 .
I pushed a commit after yesterday's discussion that changes the method name (shallowCopy) and ensures that we also recreate mutable objects in the builders.
Can you take another look ?

colings86

LGTM, thanks for making the changes

This change adds a shallow copy method for aggregation builders. This method returns a copy of the builder replacing the factoriesBuilder and metaDada This method is used when the builder is rewritten (AggregationBuilder#rewrite) in order to make sure that we create a new instance of the parent builder when sub aggregations are rewritten. Relates elastic#27782

This change adds a shallow copy method for aggregation builders. This method returns a copy of the builder replacing the factoriesBuilder and metaDada This method is used when the builder is rewritten (AggregationBuilder#rewrite) in order to make sure that we create a new instance of the parent builder when sub aggregations are rewritten. Relates #27782

This commit adds a test to check that the rewrite of a sub-aggregation triggers a copy of the parent aggregation. Relates elastic#28430 Closes elastic#27782

This commit adds a test to check that the rewrite of a sub-aggregation triggers a copy of the parent aggregation. Relates #28430 Closes #27782

jimczi added >bug :Analytics/Aggregations Aggregations >breaking-java v7.0.0 v6.3.0 labels Jan 29, 2018

jimczi requested a review from colings86 January 29, 2018 22:23

colings86 reviewed Jan 30, 2018

View reviewed changes

jimczi force-pushed the bugs/agg_builder_clone branch from 64283f5 to f1ee92a Compare January 31, 2018 09:59

jimczi changed the title ~~Add a clone method to all aggregation builders~~ Add a shallow copy method to aggregation builders Jan 31, 2018

colings86 approved these changes Jan 31, 2018

View reviewed changes

jimczi added 2 commits January 31, 2018 16:59

fix check style in aggs module

238fe67

jimczi force-pushed the bugs/agg_builder_clone branch from 018bbea to 238fe67 Compare January 31, 2018 16:00

jimczi merged commit dd40b98 into elastic:master Feb 1, 2018

jimczi deleted the bugs/agg_builder_clone branch February 1, 2018 08:22

jimczi mentioned this pull request Feb 2, 2018

Add a test for sub-aggregations rewrite #28491

Merged

jimczi added a commit that referenced this pull request Feb 2, 2018

Add a test for sub-aggregations rewrite (#28491)

88f4c1c

This commit adds a test to check that the rewrite of a sub-aggregation triggers a copy of the parent aggregation. Relates #28430 Closes #27782

jimczi added a commit that referenced this pull request Feb 2, 2018

Add a test for sub-aggregations rewrite (#28491)

1182774

This commit adds a test to check that the rewrite of a sub-aggregation triggers a copy of the parent aggregation. Relates #28430 Closes #27782

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a shallow copy method to aggregation builders #28430

Add a shallow copy method to aggregation builders #28430

jimczi commented Jan 29, 2018 •

edited

Loading

jimczi commented Jan 29, 2018 •

edited

Loading

colings86 left a comment

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

colings86 Jan 30, 2018

jimczi commented Jan 31, 2018

colings86 left a comment

Add a shallow copy method to aggregation builders #28430

Add a shallow copy method to aggregation builders #28430

Conversation

jimczi commented Jan 29, 2018 • edited Loading

jimczi commented Jan 29, 2018 • edited Loading

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Jan 31, 2018

colings86 left a comment

Choose a reason for hiding this comment

jimczi commented Jan 29, 2018 •

edited

Loading

jimczi commented Jan 29, 2018 •

edited

Loading