Copilot commented on code in PR #2517:
URL: https://github.com/apache/phoenix/pull/2517#discussion_r3397492470
##########
phoenix-core-client/src/main/java/org/apache/phoenix/compile/StatementContext.java:
##########
@@ -137,6 +143,11 @@ public StatementContext(StatementContext context) {
this.totalSegmentsValue = context.totalSegmentsValue;
this.hasRowSizeFunction = context.hasRowSizeFunction;
this.hasRawRowSizeFunction = context.hasRawRowSizeFunction;
+ this.appliedRewrites = context.appliedRewrites;
Review Comment:
`collectAppliedRewrites` claims to preserve first-occurrence order across
sub-contexts, but `subStatementContexts` is a plain hash set here, so iteration
order (and thus the emitted `REWRITE ...` order) is nondeterministic and can be
flaky across JVM runs. Using an insertion-ordered set fixes this while keeping
dedupe semantics.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/compile/StatementContext.java:
##########
@@ -199,6 +210,11 @@ public StatementContext(PhoenixStatement statement,
ColumnResolver resolver, Bin
this.retryingPersistentCache = Maps.<Long, Boolean> newHashMap();
this.hasFirstValidResult = new AtomicBoolean(false);
this.subStatementContexts = Sets.newHashSet();
+ this.appliedRewrites = new ArrayList<>();
Review Comment:
Same ordering issue as in the copy constructor: initializing
`subStatementContexts` with a hash set makes rewrite aggregation order
nondeterministic. Use an insertion-ordered set so top-of-plan `REWRITE` lines
are stable.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##########
@@ -201,26 +201,39 @@ private List<QueryPlan> getApplicablePlans(QueryPlan
dataPlan, PhoenixStatement
if (replacement != null) {
select = rewriteQueryWithIndexReplacement(statement.getConnection(),
resolver, select,
- replacement);
+ replacement, dataPlan.getContext());
}
}
// Re-compile the plan with option "optimizeSubquery" turned on, so that
enclosed
- // sub-queries can be optimized recursively.
+ // sub-queries can be optimized recursively. Carry forward the top-of-plan
rewrite breadcrumbs
+ // recorded on the data plan's context so they survive the recompile onto
the optimized plan's
+ // context.
QueryCompiler compiler = new QueryCompiler(statement, select,
FromCompiler.getResolverForQuery(select, statement.getConnection()),
targetColumns,
parallelIteratorFactory, dataPlan.getContext().getSequenceManager(),
true, true, dataPlans,
- dataPlan.getContext());
+ dataPlan.getContext()).withRewriteContext(dataPlan.getContext());
return Collections.singletonList(compiler.compile());
}
private static boolean isPartialIndexUsable(SelectStatement select,
QueryPlan dataPlan,
PTable index) throws SQLException {
StatementContext context = new StatementContext(dataPlan.getContext());
context.setResolver(FromCompiler.getResolver(dataPlan.getTableRef()));
- return WhereCompiler.contains(
- index.getIndexWhereExpression(dataPlan.getContext().getConnection()),
- WhereCompiler.transformDNF(select.getWhere(), context));
+ boolean usable =
+
WhereCompiler.contains(index.getIndexWhereExpression(dataPlan.getContext().getConnection()),
+ WhereCompiler.transformDNF(select.getWhere(), context));
+ StatementContext rootContext = dataPlan.getContext();
+ String tableName = dataPlan.getTableRef().getTable().getName().getString();
+ String indexName = index.getName().getString();
+ // Dedupe so the breadcrumb is recorded once per table and index pair even
when the optimizer
+ // scores the same index across multiple candidate paths.
+ if (rootContext.markPartialIndexChecked(tableName, indexName)) {
+ rootContext.addAppliedRewrite(usable
+ ? "PARTIAL INDEX APPLICABLE"
+ : "PARTIAL INDEX NOT APPLICABLE -- index WHERE not implied by query
WHERE");
+ }
Review Comment:
The partial-index breadcrumb text does not include which index was
evaluated. Since rewrites are later deduped by string, multiple partial indexes
can collapse into a single ambiguous `PARTIAL INDEX APPLICABLE/NOT APPLICABLE`
line, losing per-index information. Include the index name in the breadcrumb so
it remains meaningful and distinct (and won’t be deduped away).
--
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]
