[BugFix] Fix PushDownContext shallow copy bug

[songkant-aws]

Description

PushdownContext.clone() / cloneWithoutSort() / cloneForAggregate() replays add(operation) to construct new context. Aggregation operations reuses the same AggPushDownAction instance in both old and new context due to shallow copy side effect happened during cloning PushdownContext. The following pushdown actions from other equivalent RelSubsets will modify this shared mutable action object, which could contaminate previous context's agg states.

Related Issues

Resolves #5125

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f36c20a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [437]: for MultiTermsAggregationBuilder

🔀 Multiple PR themes Sub-PR theme: Introduce immutable AggSpec to fix shallow copy bug in PushDownContext Relevant files: opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggSpec.java opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/PushDownContext.java opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java Sub-PR theme: Refactor AggPushDownAction: extract copy helpers and fix histogram/date-histogram options Relevant files: opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java

⚡ Recommended focus areas for review Rebuild Cost The build() method in AggSpec re-runs AggregateAnalyzer.analyze() and replays all pushdown operations (sort, measure sort, rare top, limit) every time it is called. If build() is called multiple times per query execution (e.g., once per RelSubset evaluation), this could be a significant performance regression compared to the previous approach of mutating a shared builder. Verify that build() is only called once per final plan execution. public Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> build() { try { AggregateAnalyzer.AggregateBuilderHelper helper = new AggregateAnalyzer.AggregateBuilderHelper( rowType, fieldTypes, cluster, bucketNullable, queryBucketSize); Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> builderAndParser = AggregateAnalyzer.analyze(aggregate, project, outputFields, helper); AggPushDownAction temp = new AggPushDownAction( builderAndParser, extendedTypeMapping, new ArrayList<>(initialBucketNames)); if (bucketSortCollations != null) { temp.pushDownSortIntoAggBucket(bucketSortCollations, bucketSortFieldNames); } if (measureSortCollations != null) { temp.rePushDownSortAggMeasure(measureSortCollations, measureSortFieldNames); } if (rareTopDigest != null) { temp.rePushDownRareTop(rareTopDigest); } if (bucketSize != null) { temp.pushDownLimitIntoBucketSize(bucketSize); } return temp.getBuilderAndParser(); } catch (AggregateAnalyzer.ExpressionNotAnalyzableException e) { throw new OpenSearchRequestBuilder.PushDownUnSupportedException( "Cannot materialize aggregation pushdown", e); } } Mutable Bucket Reuse In pushDownSortIntoCompositeBucket, the existing CompositeValuesSourceBuilder instances from compositeAggBuilder.sources() are reused directly in newBuckets (with bucket.order(order) and bucket.missingOrder(missingOrder) mutating them in place). Since AggSpec.build() re-creates the composite builder from scratch via AggregateAnalyzer.analyze(), this is safe within a single build() call. However, if the original CompositeAggregationBuilder sources are shared references, mutations could still propagate. Confirm that AggregateAnalyzer.analyze() always produces fresh builder instances. @Nullable private static Integer inferBucketSize(@Nullable AggregationBuilder rootBuilder) { AggregationBuilder builder = unwrapNestedBuilder(rootBuilder); if (builder instanceof CompositeAggregationBuilder composite) { return composite.size(); } if (builder instanceof TermsAggregationBuilder terms) { return terms.size(); } if (builder instanceof MultiTermsAggregationBuilder multiTerms) { return multiTerms.size(); } if (builder instanceof TopHitsAggregationBuilder topHits) { return topHits.size(); } return null; } @Nullable private static AggregationBuilder unwrapNestedBuilder(@Nullable AggregationBuilder rootBuilder) { if (rootBuilder instanceof NestedAggregationBuilder nested && !nested.getSubAggregations().isEmpty()) { return nested.getSubAggregations().iterator().next(); } return rootBuilder; } } Clone Correctness The clone() method now iterates and re-adds each operation, which calls operation.action().pushOperation(this, operation) for each one. This means side effects of pushOperation (e.g., setting isAggregatePushed, updating startFrom) are replayed. Verify that replaying these side effects during cloning is always correct and does not produce different state than the original context (e.g., for LIMIT operations that increment startFrom). public PushDownContext clone() { PushDownContext newContext = new PushDownContext(osIndex); for (PushDownOperation operation : this) { newContext.add(operation); } newContext.aggSpec = aggSpec; return newContext; Limit Logic Change The new canEnforceLimit condition introduces alreadyBoundedByCurrentBucketSize and alreadyEnforcedByExistingLimit as additional cases that allow limit push-down to proceed. The old code only checked canUpdate. Verify that the new conditions do not allow incorrect limit push-downs, particularly the alreadyBoundedByCurrentBucketSize path where canUpdate may be false but the limit is still accepted (potentially with offset > 0 blocked by the !canUpdate && offset > 0 guard). boolean canUpdateBuilder = aggSpec.canPushDownLimitIntoBucketSize(totalSize); boolean alreadyBoundedByCurrentBucketSize = aggSpec.getBucketSize() != null && totalSize <= aggSpec.getBucketSize(); boolean alreadyEnforcedByExistingLimit = pushDownContext.isLimitPushed() && !canReduceEstimatedRowsCount; boolean canEnforceLimit = aggSpec.isCompositeAggregation() || canUpdateBuilder || alreadyBoundedByCurrentBucketSize || alreadyEnforcedByExistingLimit || (aggSpec.isSingleRowAggregation() && offset == 0); // Push down the limit into the aggregation bucket in advance to detect whether the limit // can update the aggregation builder boolean canUpdate = canReduceEstimatedRowsCount || canUpdateBuilder; if (!canEnforceLimit || (!canUpdate && offset > 0)) return null; CalciteLogicalIndexScan newScan = this.copyWithNewSchema(getRowType()); if (canUpdateBuilder) { newScan.pushDownContext.setAggSpec(aggSpec.withLimit(limit + offset)); } AbstractAction action; if (newScan.pushDownContext.getAggSpec().isCompositeAggregation()) { action = (OSRequestBuilderAction) requestBuilder -> requestBuilder.pushDownLimitToRequestTotal(limit, offset); } else { action = (OSRequestBuilderAction) requestBuilder -> {}; } newScan.pushDownContext.add(PushDownType.LIMIT, new LimitDigest(limit, offset), action);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f36c20a

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix unreachable return after calendar interval setter The throw inside the catch block will never be reached because the exception is caught and then the throw is inside the same catch block — the throw is actually the fallback when the calendar interval also fails, but it's placed inside the catch for the calendar interval, which means it only executes when getIntervalAsCalendar() throws. This logic is correct, but the return statement after calendarIntervalSetter.accept(...) is unreachable if an exception is thrown. However, the real issue is that if getIntervalAsCalendar() succeeds but then calendarIntervalSetter.accept() throws, the exception will be swallowed and re-thrown as PushDownUnSupportedException. The return after the setter call should be outside the catch block to avoid this confusion. Restructure so the throw only happens when both intervals fail. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [297-303] try { calendarIntervalSetter.accept(source.getIntervalAsCalendar()); - return; } catch (IllegalArgumentException | IllegalStateException ignored) { throw new OpenSearchRequestBuilder.PushDownUnSupportedException( "Cannot copy interval for date histogram bucket " + source.name()); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the return statement inside the try block after calendarIntervalSetter.accept() is redundant/confusing, and the improved code removes it. However, the logic is functionally equivalent since the return after the setter is only reached if no exception is thrown, and the catch block only executes on exception. The improved code is slightly cleaner but the original is not actually broken.	Low
	Avoid repeated aggregation rebuild on each request creation The aggSpec.build() call in createRequestBuilder() re-runs AggregateAnalyzer.analyze and replays all push-down operations every time a request builder is created. If createRequestBuilder() is called multiple times (e.g., for retries or pagination), this could cause performance issues or subtle bugs if the analysis is non-deterministic. Consider caching the result of aggSpec.build() or ensuring it is only called once. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/PushDownContext.java [213-216] if (aggSpec != null) { - newRequestBuilder.pushDownAggregation(aggSpec.build()); + Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> builtAgg = aggSpec.build(); + newRequestBuilder.pushDownAggregation(builtAgg); newRequestBuilder.pushTypeMapping(aggSpec.getExtendedTypeMapping()); } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about aggSpec.build() being called multiple times, potentially causing performance issues. However, the improved_code only extracts the result to a local variable within the same method call, which doesn't actually cache it across multiple createRequestBuilder() invocations, making the fix incomplete for the stated concern.	Low
	Ensure plugin setting reset runs first in teardown The teardown disables the Calcite plugin after deleting the index, but if the index deletion fails, the plugin setting may not be reset. Consider placing the settings reset before the index deletion to ensure the plugin is always disabled regardless of index deletion success. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5125.yml [38-47] teardown: - - do: - indices.delete: - index: issue5125 - ignore_unavailable: true - do: query.settings: body: transient: plugins.calcite.enabled: false + - do: + indices.delete: + index: issue5125 + ignore_unavailable: true Suggestion importance[1-10]: 4 __ Why: The suggestion has merit as resetting the plugins.calcite.enabled setting before deleting the index ensures cleanup happens regardless of index deletion success. However, since ignore_unavailable: true is already set on the index deletion, failures are already handled gracefully, making this a minor ordering improvement rather than a critical fix.	Low
	Document ordering constraint for withoutBucketSort usage In withoutBucketSort(), the bucketNames field is reset to initialBucketNames, which correctly reverts the bucket ordering. However, measureSortCollations and measureSortFieldNames are preserved in the new AggSpec. If a measure sort was applied after a bucket sort, removing the bucket sort should not affect the measure sort, so this is fine. But the kind field is not reset — if withSortMeasure was called and changed kind to measureSortTarget, calling withoutBucketSort afterward would retain the changed kind. Verify that withoutBucketSort is only called before withSortMeasure in the planning pipeline, or reset kind appropriately. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggSpec.java [223-249] public AggSpec withoutBucketSort() { if (bucketSortCollations == null) { return this; } + // Note: kind is not reset here; withoutBucketSort should only be called + // before withSortMeasure to avoid stale kind state. return new AggSpec( - ... + aggregate, + project, + outputFields, + rowType, + fieldTypes, + cluster, + bucketNullable, + queryBucketSize, + extendedTypeMapping, initialBucketNames, initialBucketNames, - ... + scriptCount, + kind, + measureSortTarget, + rareTopSupported, + null, + null, + measureSortCollations, + measureSortFieldNames, + rareTopDigest, + bucketSize); +} Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a comment to the existing code without changing any logic, and the improved_code is essentially the same as the existing_code with a comment added. This is a documentation-only suggestion with minimal impact.	Low

---

Previous suggestions

Suggestions up to commit 7df5f94

Category	Suggestion	Impact
General	Clear bucket sort when transitioning to rare-top aggregation In withRareTop, the new AggSpec is constructed with kind set to AggKind.RARE_TOP and measureSortTarget set to null. However, bucketSortCollations and bucketSortFieldNames are preserved from the current spec. If a bucket sort was previously applied, it would be carried over into the rare-top spec and re-applied during build(). This may produce incorrect query behavior since rare-top aggregations don't support composite bucket sort ordering. Consider clearing bucketSortCollations and bucketSortFieldNames when transitioning to RARE_TOP. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggSpec.java [285-311] public AggSpec withRareTop(RareTopDigest digest) { if (kind != AggKind.COMPOSITE || !rareTopSupported) { throw new OpenSearchRequestBuilder.PushDownUnSupportedException("Cannot pushdown " + digest); } return new AggSpec( - ... + aggregate, + project, + outputFields, + rowType, + fieldTypes, + cluster, + bucketNullable, + queryBucketSize, + extendedTypeMapping, + initialBucketNames, + bucketNames, + scriptCount, + AggKind.RARE_TOP, + null, + rareTopSupported, + null, // clear bucket sort collations for rare-top + null, // clear bucket sort field names for rare-top + measureSortCollations, + measureSortFieldNames, + digest, digest.byList().isEmpty() ? digest.number() : DEFAULT_MAX_BUCKETS); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern that bucketSortCollations and bucketSortFieldNames are preserved when transitioning to RARE_TOP, which could cause incorrect behavior during build() since rare-top aggregations don't support composite bucket sort. However, the actual code in withRareTop does pass bucketSortCollations and bucketSortFieldNames through, so this is a real potential issue worth addressing.	Low
	Set aggSpec before adding operations in clone The clone() method iterates over this (which uses the queue iterator) and calls newContext.add(operation) for each operation. The add method calls operation.action().pushOperation(this, operation) which may have side effects on newContext (e.g., setting isAggregatePushed, updating startFrom). However, aggSpec is set after all operations are added, which means if any operation's pushOperation tries to access aggSpec on newContext, it would be null. Since aggSpec is now set separately from the AGGREGATION operation's add() call (the action is a no-op lambda), this ordering should be safe. But it's fragile — consider setting aggSpec before adding operations or documenting this dependency. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/PushDownContext.java [51-58] @Override public PushDownContext clone() { PushDownContext newContext = new PushDownContext(osIndex); + newContext.aggSpec = aggSpec; // Set before adding operations to avoid null access for (PushDownOperation operation : this) { newContext.add(operation); } - newContext.aggSpec = aggSpec; return newContext; } Suggestion importance[1-10]: 4 __ Why: The suggestion to set aggSpec before iterating operations in clone() is a valid defensive improvement to avoid potential null access if any pushOperation side effect ever accesses aggSpec. However, since the AGGREGATION action is a no-op lambda in the new design, the risk is currently low, making this a minor maintainability improvement.	Low
	Fix teardown order to reset settings before deleting index The teardown disables the Calcite plugin, but if the test fails mid-execution, the plugin setting may not be reset. Consider also adding the settings reset to the setup block or ensuring the teardown order is correct (delete index first, then reset settings) to avoid leaving the cluster in a bad state for subsequent tests. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5125.yml [38-47] teardown: - - do: - indices.delete: - index: issue5125 - ignore_unavailable: true - do: query.settings: body: transient: plugins.calcite.enabled: false + - do: + indices.delete: + index: issue5125 + ignore_unavailable: true Suggestion importance[1-10]: 3 __ Why: The suggestion to reorder teardown steps (reset settings before deleting index) is a minor improvement for test hygiene, but the original order (delete index first, then reset settings) is also valid and commonly used. The teardown block runs regardless of test failure in YAML REST tests, so the concern about "failing mid-execution" is less critical here.	Low

Suggestions up to commit 9fe5192

Category	Suggestion	Impact
General	Preserve root cause in exception chaining The second catch block catches IllegalArgumentException | IllegalStateException but then unconditionally throws a PushDownUnSupportedException. This means if getIntervalAsCalendar() itself throws (e.g., because neither interval is set), the original exception is silently swallowed and replaced with a less informative message. Consider including the caught exception as the cause to preserve the root cause for debugging. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [287-304] private static void copyDateHistogramInterval( DateHistogramValuesSourceBuilder source, Consumer<DateHistogramInterval> fixedIntervalSetter, Consumer<DateHistogramInterval> calendarIntervalSetter) { try { fixedIntervalSetter.accept(source.getIntervalAsFixed()); return; } catch (IllegalArgumentException | IllegalStateException ignored) { // Fallback to calendar interval. } try { calendarIntervalSetter.accept(source.getIntervalAsCalendar()); return; - } catch (IllegalArgumentException | IllegalStateException ignored) { + } catch (IllegalArgumentException | IllegalStateException e) { throw new OpenSearchRequestBuilder.PushDownUnSupportedException( - "Cannot copy interval for date histogram bucket " + source.name()); + "Cannot copy interval for date histogram bucket " + source.name(), e); } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the caught exception in the second catch block is silently swallowed, losing the root cause. Including the caught exception as the cause of PushDownUnSupportedException improves debuggability, and the new constructor for this purpose was added in this same PR.	Low
	Preserve bucket size for all measure sort targets When withSortMeasure is called, the new AggSpec sets kind to measureSortTarget and measureSortTarget to null. However, bucketSize is set to resizedBucketSize, which is null for non-TERMS/MULTI_TERMS targets (e.g., DATE_HISTOGRAM, HISTOGRAM). This means canPushDownLimitIntoBucketSize will always return false after a measure sort push-down for those types, even if a bucket size was previously set. Consider preserving bucketSize for all measure sort targets, or explicitly document why it should be dropped. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggSpec.java [242-251] public AggSpec withSortMeasure(List<RelFieldCollation> collations, List<String> fieldNames) { if (kind != AggKind.COMPOSITE || measureSortTarget == null) { throw new OpenSearchRequestBuilder.PushDownUnSupportedException( "Cannot pushdown sort aggregate measure"); } Integer resizedBucketSize = switch (measureSortTarget) { case TERMS, MULTI_TERMS -> bucketSize; + case DATE_HISTOGRAM, HISTOGRAM -> bucketSize; // preserve bucket size for histogram types default -> null; }; Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern that bucketSize is dropped for DATE_HISTOGRAM and HISTOGRAM targets after withSortMeasure, which could prevent limit push-down from working correctly for those types. However, the improved_code doesn't fully resolve the issue (it just adds more cases to the switch without changing the default null).	Low
	Avoid redundant re-analysis on repeated builder creation The aggSpec.build() call in createRequestBuilder() re-runs AggregateAnalyzer.analyze and re-applies all push-down operations every time a request builder is created. If createRequestBuilder() is called multiple times (e.g., for cost estimation or plan copying), this could be expensive or produce inconsistent results if the underlying state changes. Consider caching the result of aggSpec.build() or ensuring createRequestBuilder() is only called once per execution. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/PushDownContext.java [213-216] if (aggSpec != null) { - newRequestBuilder.pushDownAggregation(aggSpec.build()); + // Note: aggSpec.build() re-analyzes and re-applies all push-downs; ensure this is only called once per execution. + Pair<List<AggregationBuilder>, OpenSearchAggregationResponseParser> builtAgg = aggSpec.build(); + newRequestBuilder.pushDownAggregation(builtAgg); newRequestBuilder.pushTypeMapping(aggSpec.getExtendedTypeMapping()); } Suggestion importance[1-10]: 3 __ Why: The concern about repeated aggSpec.build() calls is valid for performance, but the improved_code only extracts the result to a local variable without actually caching it across calls. The suggestion is more of a note/verification than a concrete fix.	Low
	Verify sort mutation is applied on fresh builder The pushDownSortIntoTermsBucket method in AggPushDownAction mutates the termsAggBuilder in place, but since AggSpec.build() reconstructs the aggregation from scratch each time via AggregateAnalyzer.analyze, this mutation is applied to a freshly created builder and is not persisted across calls. This is consistent with the new design, but the method body is effectively a no-op since the sort is re-applied on each build() call. Verify that the TermsAggregationBuilder sort order is correctly applied during build() and that the method is not relying on side effects from a previous call. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggSpec.java [500-505] private void pushDownSortIntoTermsBucket( AggregationBuilder original, TermsAggregationBuilder termsAggBuilder, List<RelFieldCollation> collations) { termsAggBuilder.order(BucketOrder.key(!collations.getFirst().getDirection().isDescending())); + // The builder is freshly created in build(), so this mutation is correctly applied each time. } Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify behavior and adds a comment; the improved_code is functionally identical to existing_code. The design is intentional since build() reconstructs from scratch each time, so this is not a real issue.	Low
Possible issue	Fix mismatched index name in expected output The expected output references opensearch-sql_test_index_bank, but the integration test in 5125.yml creates and queries the issue5125 index. This mismatch will cause the explain test to fail or validate against the wrong index. Ensure both files reference the same index. integ-test/src/test/resources/expectedOutput/calcite/explain_agg_consecutive_sorts_issue_5125.yaml [11] -CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[AGGREGATION->rel#:LogicalAggregate.NONE.[](input=RelSubset#,group={0},c=COUNT()), PROJECT->[c, gender], SORT->[1 DESC LAST], LIMIT->10000], ... +CalciteEnumerableIndexScan(table=[[OpenSearch, issue5125]], PushDownContext=[[AGGREGATION->rel#:LogicalAggregate.NONE.[](input=RelSubset#,group={0},c=COUNT()), PROJECT->[c, gender], SORT->[1 DESC LAST], LIMIT->10000], ... Suggestion importance[1-10]: 3 __ Why: The suggestion points out a potential mismatch between opensearch-sql_test_index_bank in the explain output and issue5125 in the REST test. However, these are likely two separate tests — the explain test may intentionally use opensearch-sql_test_index_bank as a pre-existing test index with gender field, while 5125.yml creates its own issue5125 index. Without more context on how the explain test is invoked, this may be a false positive.	Low
	Verify test index consistency across files The test query uses source=issue5125 but the index created in setup is issue5125, while the expected output YAML references opensearch-sql_test_index_bank. Verify that the test query references the correct index and that the expected output file matches the actual index used in this test. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5125.yml [62] +query: source=issue5125 | stats count() as c by gender | sort gender | sort - gender - Suggestion importance[1-10]: 2 __ Why: The 5125.yml test uses issue5125 index and the explain_agg_consecutive_sorts_issue_5125.yaml uses opensearch-sql_test_index_bank. These are likely two separate test files testing different aspects (one is a REST API test, the other is an explain plan test). The improved_code is identical to existing_code, indicating no actual fix is proposed, just a verification request.	Low

Suggestions up to commit 21f8940

Category	Suggestion	Impact
Possible issue	Deep-copy all composite buckets, not just sorted ones The remaining (non-sorted) buckets are added to newBuckets as direct references from the original buckets list without copying, while the sorted buckets are properly deep-copied via copyCompositeBucket. This inconsistency means the non-sorted buckets are still shared references and could be mutated by other PushDownContext instances, defeating the purpose of this bug fix. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [664-671] buckets.stream() .map(CompositeValuesSourceBuilder::name) .filter(name -> !selected.contains(name)) .forEach( name -> { - newBuckets.add(buckets.get(bucketNames.indexOf(name))); + newBuckets.add(copyCompositeBucket(buckets.get(bucketNames.indexOf(name)))); newBucketNames.add(name); }); Suggestion importance[1-10]: 8 __ Why: This is a real correctness issue - the non-sorted buckets are added as direct references from the original buckets list, while sorted buckets are deep-copied via copyCompositeBucket. This inconsistency means shared bucket references could be mutated across different PushDownContext instances, which is exactly the bug this PR is trying to fix.	Medium
	Verify index consistency between test and expected output The integration test uses opensearch-sql_test_index_bank in the expected output file (explain_agg_consecutive_sorts_issue_5125.yaml) but the REST test creates and queries the issue5125 index. Verify that the expected output file references the correct index, or that the REST test is querying the intended index to ensure the test validates the actual bug fix scenario. integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5125.yml [62] +query: source=issue5125 | stats count() as c by gender | sort gender | sort - gender - Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about the mismatch between the issue5125 index used in the REST test and opensearch-sql_test_index_bank referenced in the expected output YAML. However, the existing_code and improved_code are identical, meaning no actual fix is proposed — it only asks the user to verify. This is a legitimate observation worth investigating but doesn't constitute a code fix.	Low
General	Preserve root cause when rethrowing interval copy exception The second catch block catches ignored but then immediately throws a new exception, meaning the caught exception is silently discarded and the root cause is lost. The throw statement should be outside the catch block (in a final else or after both try blocks), or the caught exception should be chained as the cause of the new exception to preserve the original error context. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [422-439] private static void copyDateHistogramInterval( DateHistogramValuesSourceBuilder source, Consumer<DateHistogramInterval> fixedIntervalSetter, Consumer<DateHistogramInterval> calendarIntervalSetter) { try { fixedIntervalSetter.accept(source.getIntervalAsFixed()); return; } catch (IllegalArgumentException | IllegalStateException ignored) { // Fallback to calendar interval. } try { calendarIntervalSetter.accept(source.getIntervalAsCalendar()); - return; - } catch (IllegalArgumentException | IllegalStateException ignored) { + } catch (IllegalArgumentException | IllegalStateException e) { throw new OpenSearchRequestBuilder.PushDownUnSupportedException( - "Cannot copy interval for date histogram bucket " + source.name()); + "Cannot copy interval for date histogram bucket " + source.name(), e); } } Suggestion importance[1-10]: 5 __ Why: The suggestion is valid - the caught exception e should be chained as the cause of the new PushDownUnSupportedException to preserve the root cause. However, the PushDownUnSupportedException constructor may not accept a cause parameter, and this is a minor diagnostic improvement rather than a correctness issue.	Low

Suggestions up to commit 62df8d2

Category	Suggestion	Impact
Possible issue	Copy remaining composite buckets to avoid shared mutable state The remaining (non-sorted) buckets are added directly from the original buckets list without copying, meaning they still share the same mutable CompositeValuesSourceBuilder instances. This can cause the same shallow-copy bug that this PR is trying to fix. Each remaining bucket should also be copied via copyCompositeBucket. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [610-617] buckets.stream() .map(CompositeValuesSourceBuilder::name) .filter(name -> !selected.contains(name)) .forEach( name -> { - newBuckets.add(buckets.get(bucketNames.indexOf(name))); + newBuckets.add(copyCompositeBucket(buckets.get(bucketNames.indexOf(name)))); newBucketNames.add(name); }); Suggestion importance[1-10]: 8 __ Why: The non-sorted buckets are added directly from the original buckets list without copying, which means they still share mutable CompositeValuesSourceBuilder instances. This is the exact bug the PR aims to fix, and the copyCompositeBucket method is already available for this purpose.	Medium
General	Fix incorrect null return when limit already satisfies bucket size When size >= bucketSize, resizeAggregationForLimit returns null, which causes pushDownLimitIntoBucketSize to return false (cannot push down). However, canPushDownLimitIntoBucketSize returns false in the same case. This is consistent, but the pushDownLimitIntoBucketSize caller in CalciteLogicalIndexScan uses canPushDownLimitIntoBucketSize as a read-only probe and then calls pushDownLimitIntoBucketSize to actually mutate — if resizeAggregationForLimit returns the original builder unchanged (e.g. for LeafOnly), replaceRootBuilder is skipped correctly. However, when size >= bucketSize, returning null means the limit is NOT pushed down even though it could be considered already satisfied. Consider returning the original builder (no resize needed) instead of null when size >= bucketSize to correctly signal that push-down is possible (the bucket is already small enough). opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [702-713] private AggregationBuilder resizeAggregationForLimit(AggregationBuilder builder, int size) { Integer bucketSize = getBucketSize(builder); if (bucketSize != null) { - return size < bucketSize ? copyAndResizeBucketBuilder(builder, size) : null; + return size < bucketSize ? copyAndResizeBucketBuilder(builder, size) : builder; } if (builder instanceof ValuesSourceAggregationBuilder.LeafOnly<?, ?>) { // all metric aggregations generate one row and are effectively already limited. return builder; } throw new OpenSearchRequestBuilder.PushDownUnSupportedException( "Unknown aggregation builder " + builder.getClass().getSimpleName()); } Suggestion importance[1-10]: 5 __ Why: The suggestion argues that returning null when size >= bucketSize incorrectly signals that push-down failed, but looking at the caller in CalciteLogicalIndexScan, canPushDownLimitIntoBucketSize is used as a read-only probe and pushDownLimitIntoBucketSize is the actual mutating call. The behavior is intentional — when the bucket is already small enough, no resize is needed and returning null (no-op) is consistent with the existing canPushDownLimitIntoBucketSize returning false. The suggestion may introduce unintended behavior changes.	Low
	Throw on unrecognized builder type instead of silently sharing When the builder type is not recognized (falls through all instanceof checks), the original builder is returned as-is without copying. This silently shares the mutable builder between the original and cloned AggPushDownAction, which is the exact bug this PR aims to fix. An exception should be thrown for unrecognized types to make the problem visible, or at minimum a comment should document why sharing is safe. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/AggPushDownAction.java [136-153] private static AggregationBuilder copyAggregationBuilder(AggregationBuilder builder) { if (builder instanceof CompositeAggregationBuilder composite) { return copyCompositeAggregationBuilder(composite); } if (builder instanceof TermsAggregationBuilder terms) { return copyTermsAggregationBuilder(terms); } if (builder instanceof MultiTermsAggregationBuilder multiTerms) { return copyMultiTermsAggregationBuilder(multiTerms); } if (builder instanceof TopHitsAggregationBuilder topHits) { return copyTopHitsAggregationBuilder(topHits); } if (builder instanceof NestedAggregationBuilder nested) { return copyNestedAggregationBuilder(nested); } - return builder; + throw new OpenSearchRequestBuilder.PushDownUnSupportedException( + "Cannot deep-copy unsupported AggregationBuilder type: " + builder.getClass().getSimpleName()); } Suggestion importance[1-10]: 4 __ Why: Throwing an exception for unrecognized builder types would make bugs more visible, but it could also break existing functionality if there are other builder types in use that are currently handled safely by the fall-through. The suggestion is valid as a defensive programming measure but carries risk.	Low

[github-actions]

Persistent review updated to latest commit 21f8940

[LantaoJin]

cc @qianheng-aws as proper reviewer

[qianheng-aws]

Have you ever tried lazy push-down for AggregationBuilderAction? If that approach can work, we don't need to create AggregationBuilder in the planning phase, neither need to do deep copy for AggregationBuilder then.

[qianheng-aws]

What if there is other missed or upcoming agg builder?

[qianheng-aws]

Can this be replaced with

source.shallowCopy(source, copySubAggregations(source), copyMetadataOrNull(source))

instead of defining these extended class?

[github-actions]

Persistent review updated to latest commit 9fe5192

[github-actions]

Persistent review updated to latest commit 7df5f94

[github-actions]

Persistent review updated to latest commit f36c20a