Copilot commented on code in PR #2537:
URL: https://github.com/apache/phoenix/pull/2537#discussion_r3430368676
##########
phoenix-core-client/src/main/java/org/apache/phoenix/compile/StatementContext.java:
##########
@@ -541,22 +532,50 @@ public List<String> getAppliedRewrites() {
}
/**
- * Adopt the rewrite breadcrumb accumulator (and related diagnostic state)
of another context by
- * sharing its references. Used to carry breadcrumbs recorded by the early
+ * Adopt the rewrite breadcrumb accumulator (and related diagnostic state)
of another context.
+ * Used to carry breadcrumbs recorded by the early
* {@link SubselectRewriter}/{@link SubqueryRewriter} pass on a pre-built
context across the
- * rewrite boundary onto the actual compilation context.
+ * rewrite boundary onto the actual compilation context. Fresh collections
are allocated (see
+ * {@link #copyRewriteStateFrom}) so this context and {@code source}
continue to mutate
+ * independently: after adoption the source is still read and written (e.g.
the optimizer keeps
+ * recording per-index applicability breadcrumbs on the data plan's context
and reconciles them
+ * onto the candidate plans), so sharing the backing collections by
reference would let two
+ * contexts read and write the same lists/maps/sets.
*/
public void adoptRewriteState(StatementContext source) {
- this.appliedRewrites = source.appliedRewrites;
+ copyRewriteStateFrom(source);
+ }
+
+ /**
+ * Copy the rewrite breadcrumb accumulators and related diagnostic state
from {@code source} into
+ * this context, allocating fresh collections (deep enough to cover the
nested lists/maps/sets
+ * that accumulate during compilation) so the two contexts never share
mutable backing
+ * collections. Shared by the copy constructor and {@link
#adoptRewriteState}, both of which hand
+ * the resulting context to a compile pass that continues to mutate this
state.
+ */
+ private void copyRewriteStateFrom(StatementContext source) {
+ this.appliedRewrites = new ArrayList<>(source.appliedRewrites);
this.derivedTableFlattenCount = source.derivedTableFlattenCount;
- this.appliedIndexExpressionMatches = source.appliedIndexExpressionMatches;
- this.appliedIndexExpressionPairs = source.appliedIndexExpressionPairs;
- this.functionalIndexNames = source.functionalIndexNames;
- this.partialIndexCheckedSet = source.partialIndexCheckedSet;
+ this.appliedIndexExpressionMatches = Maps.newLinkedHashMap();
+ for (Map.Entry<String, List<String>> entry :
source.appliedIndexExpressionMatches.entrySet()) {
+ this.appliedIndexExpressionMatches.put(entry.getKey(), new
ArrayList<>(entry.getValue()));
+ }
+ this.appliedIndexExpressionPairs = Maps.newLinkedHashMap();
+ for (Map.Entry<String, Map<String, String>> entry :
source.appliedIndexExpressionPairs
+ .entrySet()) {
+ this.appliedIndexExpressionPairs.put(entry.getKey(), new
LinkedHashMap<>(entry.getValue()));
+ }
+ this.functionalIndexNames = Sets.newHashSet(source.functionalIndexNames);
+ this.partialIndexCheckedSet =
Sets.newHashSet(source.partialIndexCheckedSet);
Review Comment:
Copying these sets with `Sets.newHashSet(...)` can change iteration order
compared to the source (e.g., if the source uses `LinkedHashSet`), which can
make diagnostics/EXPLAIN-related outputs or tests flaky when order is observed.
Prefer preserving encounter order by copying into `LinkedHashSet` (or using
`Sets.newLinkedHashSet(...)`) for both `functionalIndexNames` and
`partialIndexCheckedSet`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
