Copilot commented on code in PR #2513:
URL: https://github.com/apache/phoenix/pull/2513#discussion_r3392358735
##########
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ExplainTable.java:
##########
@@ -131,6 +134,27 @@ protected int getSplitCount() {
return 0;
}
+ /**
+ * The optimizer's index selection rationale for the plan this scan belongs
to, used to render the
+ * per-scan {@code INDEX} rule comment and the {@code !INDEX} rejection
comments. Returns
+ * {@code null} when the plan did not participate in optimizer index
selection (DML, DDL, and
+ * non-optimizer plans).
+ * @return the decision, or {@code null} when unavailable
+ */
Review Comment:
Javadoc lines here exceed the 100-character Checkstyle LineLength limit;
this will fail Checkstyle unless wrapped.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##########
@@ -789,9 +816,169 @@ public int compare(QueryPlan plan1, QueryPlan plan2) {
});
+ // Capture the optimizer's rationale for the chosen plan without altering
the ordering above.
+ QueryPlan winner = bestCandidates.get(0);
+ QueryPlan runnerUp =
+ bestCandidates.size() > 1 ? bestCandidates.get(1) : firstOther(plans,
winner);
+ tagComparatorRejections(winner, plans, boundRanges, state);
+ recordDecision(winner, labelComparatorRule(winner, runnerUp, boundRanges),
state);
+
return stopAtBestPlan ? bestCandidates.subList(0, 1) : bestCandidates;
}
+ /**
+ * Outcome of attempting to build an index candidate plan in {@link
#addPlan}. Carries either the
+ * successfully built {@code plan} or a {@code rejection} explaining why the
candidate index was
+ * not usable. Exactly one of the two is non-null. This discriminated-union
return keeps rejection
+ * bookkeeping out of {@code addPlan} itself (no shared mutable state); the
caller records the
+ * rejection onto its {@link DecisionState}.
+ */
+ private static final class AddPlanResult {
+ final QueryPlan plan;
+ final RejectedIndexEntry rejection;
+
+ private AddPlanResult(QueryPlan plan, RejectedIndexEntry rejection) {
+ this.plan = plan;
+ this.rejection = rejection;
+ }
+
+ static AddPlanResult success(QueryPlan plan) {
+ return new AddPlanResult(plan, null);
+ }
+
+ static AddPlanResult rejected(PTable index, String reason) {
+ return new AddPlanResult(null,
+ new RejectedIndexEntry(index.getTableName().getString(), reason));
+ }
+ }
+
+ /**
+ * Accumulates the rejected-index entries gathered while selecting an index
for a single flat
+ * query. Rejections are de-duplicated by index name with first-reason-wins
semantics, so a reason
+ * recorded earlier in the pipeline (e.g. while building the candidate) is
not overwritten by a
+ * later comparator-stage reason for the same index.
+ */
+ private static final class DecisionState {
+ private final List<RejectedIndexEntry> rejections = new ArrayList<>();
+
+ void reject(String name, String reason) {
+ for (RejectedIndexEntry existing : rejections) {
+ if (existing.getName().equals(name)) {
+ return;
+ }
+ }
+ rejections.add(new RejectedIndexEntry(name, reason));
+ }
+
+ void reject(PTable index, String reason) {
+ reject(index.getTableName().getString(), reason);
+ }
+
+ void reject(RejectedIndexEntry entry) {
+ if (entry != null) {
+ reject(entry.getName(), entry.getReason());
+ }
+ }
+
+ void rejectAll(List<PTable> indexes, String reason) {
+ for (PTable index : indexes) {
+ reject(index, reason);
+ }
+ }
+
+ List<RejectedIndexEntry> getRejections() {
+ return rejections;
+ }
+ }
+
+ /**
+ * Records the optimizer's index-selection rationale on the chosen plan and
returns it so callers
+ * can use this inline at a {@code return} site. The chosen-index name is
the plan's table name
+ * (the data table for a data-table target, otherwise the index table name).
+ */
+ private static QueryPlan recordDecision(QueryPlan winner, String rule,
DecisionState state) {
+ winner.setOptimizerDecision(
+ new
OptimizerDecision(winner.getTableRef().getTable().getTableName().getString(),
rule,
+ state == null ? null : state.getRejections()));
+ return winner;
+ }
+
+ /** First plan in {@code plans} that is not {@code exclude}, or {@code null}
if there is none. */
+ private static QueryPlan firstOther(List<QueryPlan> plans, QueryPlan
exclude) {
+ for (QueryPlan plan : plans) {
+ if (plan != exclude) {
+ return plan;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * The bound-PK-column count for {@code plan}, adjusted exactly as the
comparator in
+ * {@link #orderPlansBestToWorst} adjusts it: view-index id columns are
added back and the salt
+ * column is removed.
+ */
+ private static int adjustedBoundCount(QueryPlan plan, int boundRanges) {
+ PTable table = plan.getTableRef().getTable();
+ int boundCount = plan.getContext().getScanRanges().getBoundPkColumnCount();
+ boundCount += table.getViewIndexId() == null ? 0 : (boundRanges - 1);
+ boundCount -= plan.getContext().getScanRanges().isSalted() ? 1 : 0;
+ return boundCount;
+ }
+
+ /**
+ * Returns the {@code RULE_*} label for the chosen plan by replaying the
comparator ladder in
+ * {@link #orderPlansBestToWorst} against the runner-up and returning the
first decisive branch.
+ * Read-only: it does not reorder or mutate any plan. When there is no
runner-up the chosen plan
+ * was the only candidate. When every comparator step ties, the final
local-vs-global tie-break is
+ * what selected the winner, so {@code RULE_NON_LOCAL_PREFERRED} is the
fall-through label.
+ */
Review Comment:
This Javadoc block has lines longer than 100 characters, which violates the
repo Checkstyle LineLength rule.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##########
@@ -789,9 +816,169 @@ public int compare(QueryPlan plan1, QueryPlan plan2) {
});
+ // Capture the optimizer's rationale for the chosen plan without altering
the ordering above.
+ QueryPlan winner = bestCandidates.get(0);
+ QueryPlan runnerUp =
+ bestCandidates.size() > 1 ? bestCandidates.get(1) : firstOther(plans,
winner);
+ tagComparatorRejections(winner, plans, boundRanges, state);
+ recordDecision(winner, labelComparatorRule(winner, runnerUp, boundRanges),
state);
+
return stopAtBestPlan ? bestCandidates.subList(0, 1) : bestCandidates;
}
+ /**
+ * Outcome of attempting to build an index candidate plan in {@link
#addPlan}. Carries either the
+ * successfully built {@code plan} or a {@code rejection} explaining why the
candidate index was
+ * not usable. Exactly one of the two is non-null. This discriminated-union
return keeps rejection
+ * bookkeeping out of {@code addPlan} itself (no shared mutable state); the
caller records the
+ * rejection onto its {@link DecisionState}.
+ */
+ private static final class AddPlanResult {
+ final QueryPlan plan;
+ final RejectedIndexEntry rejection;
+
+ private AddPlanResult(QueryPlan plan, RejectedIndexEntry rejection) {
+ this.plan = plan;
+ this.rejection = rejection;
+ }
+
+ static AddPlanResult success(QueryPlan plan) {
+ return new AddPlanResult(plan, null);
+ }
+
+ static AddPlanResult rejected(PTable index, String reason) {
+ return new AddPlanResult(null,
+ new RejectedIndexEntry(index.getTableName().getString(), reason));
+ }
+ }
+
+ /**
+ * Accumulates the rejected-index entries gathered while selecting an index
for a single flat
+ * query. Rejections are de-duplicated by index name with first-reason-wins
semantics, so a reason
+ * recorded earlier in the pipeline (e.g. while building the candidate) is
not overwritten by a
+ * later comparator-stage reason for the same index.
+ */
+ private static final class DecisionState {
+ private final List<RejectedIndexEntry> rejections = new ArrayList<>();
+
+ void reject(String name, String reason) {
+ for (RejectedIndexEntry existing : rejections) {
+ if (existing.getName().equals(name)) {
+ return;
+ }
+ }
+ rejections.add(new RejectedIndexEntry(name, reason));
+ }
+
+ void reject(PTable index, String reason) {
+ reject(index.getTableName().getString(), reason);
+ }
+
+ void reject(RejectedIndexEntry entry) {
+ if (entry != null) {
+ reject(entry.getName(), entry.getReason());
+ }
+ }
+
+ void rejectAll(List<PTable> indexes, String reason) {
+ for (PTable index : indexes) {
+ reject(index, reason);
+ }
+ }
+
+ List<RejectedIndexEntry> getRejections() {
+ return rejections;
+ }
+ }
+
+ /**
+ * Records the optimizer's index-selection rationale on the chosen plan and
returns it so callers
+ * can use this inline at a {@code return} site. The chosen-index name is
the plan's table name
+ * (the data table for a data-table target, otherwise the index table name).
+ */
+ private static QueryPlan recordDecision(QueryPlan winner, String rule,
DecisionState state) {
+ winner.setOptimizerDecision(
+ new
OptimizerDecision(winner.getTableRef().getTable().getTableName().getString(),
rule,
+ state == null ? null : state.getRejections()));
+ return winner;
+ }
+
+ /** First plan in {@code plans} that is not {@code exclude}, or {@code null}
if there is none. */
+ private static QueryPlan firstOther(List<QueryPlan> plans, QueryPlan
exclude) {
+ for (QueryPlan plan : plans) {
+ if (plan != exclude) {
+ return plan;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * The bound-PK-column count for {@code plan}, adjusted exactly as the
comparator in
+ * {@link #orderPlansBestToWorst} adjusts it: view-index id columns are
added back and the salt
+ * column is removed.
+ */
+ private static int adjustedBoundCount(QueryPlan plan, int boundRanges) {
+ PTable table = plan.getTableRef().getTable();
+ int boundCount = plan.getContext().getScanRanges().getBoundPkColumnCount();
+ boundCount += table.getViewIndexId() == null ? 0 : (boundRanges - 1);
+ boundCount -= plan.getContext().getScanRanges().isSalted() ? 1 : 0;
+ return boundCount;
+ }
+
+ /**
+ * Returns the {@code RULE_*} label for the chosen plan by replaying the
comparator ladder in
+ * {@link #orderPlansBestToWorst} against the runner-up and returning the
first decisive branch.
+ * Read-only: it does not reorder or mutate any plan. When there is no
runner-up the chosen plan
+ * was the only candidate. When every comparator step ties, the final
local-vs-global tie-break is
+ * what selected the winner, so {@code RULE_NON_LOCAL_PREFERRED} is the
fall-through label.
+ */
+ private static String labelComparatorRule(QueryPlan winner, QueryPlan
runnerUp, int boundRanges) {
+ if (runnerUp == null) {
+ return OptimizerReasons.RULE_ONLY_CANDIDATE;
+ }
+ PTable winnerTable = winner.getTableRef().getTable();
+ PTable runnerUpTable = runnerUp.getTableRef().getTable();
+ if (adjustedBoundCount(winner, boundRanges) !=
adjustedBoundCount(runnerUp, boundRanges)) {
+ return OptimizerReasons.RULE_MORE_BOUND_PK_COLUMNS;
+ }
+ if (winner.getGroupBy() != null && runnerUp.getGroupBy() != null) {
+ if (winner.getGroupBy().isOrderPreserving() !=
runnerUp.getGroupBy().isOrderPreserving()) {
+ return OptimizerReasons.RULE_ORDER_PRESERVING;
+ }
+ }
+ if (winnerTable.getIndexWhere() != null && runnerUpTable.getIndexWhere()
== null) {
+ return OptimizerReasons.RULE_PARTIAL_INDEX_APPLICABLE;
+ }
+ return OptimizerReasons.RULE_NON_LOCAL_PREFERRED;
+ }
+
+ /**
+ * Tags every index candidate that lost to {@code winner} with a rejection
reason that mirrors the
+ * comparator's decisive step. Only INDEX-type candidates are tagged (the
data table is not an
+ * "index"). De-duplication in {@link DecisionState} ensures an earlier,
more specific reason
+ * recorded while building the candidate is preserved.
+ */
Review Comment:
These Javadoc lines appear to exceed the 100-character limit enforced by
Checkstyle (LineLength), which will fail the build. Wrap them to shorter lines.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ExplainTable.java:
##########
@@ -216,7 +240,21 @@ protected void explain(String prefix, List<String>
planSteps,
indexKind = null;
}
}
- planSteps.add(" INDEX " + explainIndexName + (indexKind == null ? "" :
" " + indexKind));
+ OptimizerDecision decision = getOptimizerDecision();
+ StringBuilder indexLine = new StringBuilder(" INDEX
").append(explainIndexName);
+ if (indexKind != null) {
+ indexLine.append(" ").append(indexKind);
+ }
+ if (decision != null && !isDefaultRule(decision.getRule())) {
+ indexLine.append(" /* ").append(decision.getRule()).append(" */");
+ }
+ planSteps.add(indexLine.toString());
+ if (decision != null) {
+ for (RejectedIndexEntry rejected : decision.getRejectedIndexes()) {
+ planSteps
+ .add(" /* !INDEX " + rejected.getName() + " -- " +
rejected.getReason() + " */");
+ }
Review Comment:
The rejected-index EXPLAIN line construction is very long and likely exceeds
the 100-character Checkstyle LineLength limit. Consider splitting it to keep
the build green.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##########
@@ -789,9 +816,169 @@ public int compare(QueryPlan plan1, QueryPlan plan2) {
});
+ // Capture the optimizer's rationale for the chosen plan without altering
the ordering above.
+ QueryPlan winner = bestCandidates.get(0);
+ QueryPlan runnerUp =
+ bestCandidates.size() > 1 ? bestCandidates.get(1) : firstOther(plans,
winner);
+ tagComparatorRejections(winner, plans, boundRanges, state);
+ recordDecision(winner, labelComparatorRule(winner, runnerUp, boundRanges),
state);
+
return stopAtBestPlan ? bestCandidates.subList(0, 1) : bestCandidates;
}
+ /**
+ * Outcome of attempting to build an index candidate plan in {@link
#addPlan}. Carries either the
+ * successfully built {@code plan} or a {@code rejection} explaining why the
candidate index was
+ * not usable. Exactly one of the two is non-null. This discriminated-union
return keeps rejection
+ * bookkeeping out of {@code addPlan} itself (no shared mutable state); the
caller records the
+ * rejection onto its {@link DecisionState}.
+ */
Review Comment:
Several Javadoc lines exceed the 100-character Checkstyle limit (see
checker.xml LineLength). This will fail the build unless wrapped.
##########
phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainPlanTestUtil.java:
##########
@@ -190,6 +191,72 @@ public ExplainPlanAssert indexKind(String expected) {
return this;
}
+ /**
+ * Assert the optimizer's index-selection rule label, e.g. one of the
+ * {@code OptimizerReasons.RULE_*} constants (null when the plan did not
participate in
+ * optimizer index selection).
+ */
+ public ExplainPlanAssert indexRule(String expected) {
+ assertEquals(at("indexRule"), expected, attributes.getIndexRule());
+ return this;
+ }
+
+ /**
+ * Assert the index-selection rule label starts with {@code prefix}.
Useful for the functional
+ * index {@code "matches <expr>"} rule whose suffix is
expression-dependent.
+ */
+ public ExplainPlanAssert indexRuleStartsWith(String prefix) {
+ String actual = attributes.getIndexRule();
+ assertNotNull(at("indexRule") + " must not be null", actual);
+ assertTrue(
+ at("indexRule") + " expected to start with '" + prefix + "' but was '"
+ actual + "'",
+ actual.startsWith(prefix));
+ return this;
+ }
+
+ /** Assert the number of rejected index candidates recorded for this plan.
*/
+ public ExplainPlanAssert indexRejectedCount(int expected) {
+ List<RejectedIndexEntry> rejected = attributes.getIndexRejected();
+ int actual = rejected == null ? 0 : rejected.size();
+ assertEquals(at("indexRejected.size"), expected, actual);
+ return this;
+ }
+
+ /** Assert the i-th rejected index candidate's name and reason. */
+ public ExplainPlanAssert indexRejected(int i, String name, String reason) {
+ List<RejectedIndexEntry> rejected = attributes.getIndexRejected();
+ assertNotNull(at("indexRejected") + " must not be null", rejected);
+ assertTrue(at("indexRejected") + " has no index " + i + " (size=" +
rejected.size() + ")",
+ i >= 0 && i < rejected.size());
+ RejectedIndexEntry entry = rejected.get(i);
+ assertEquals(at("indexRejected[" + i + "].name"), name, entry.getName());
+ assertEquals(at("indexRejected[" + i + "].reason"), reason,
entry.getReason());
+ return this;
+ }
+
+ /** Assert that no index candidates were rejected (null or empty list). */
+ public ExplainPlanAssert indexRejectedNone() {
+ List<RejectedIndexEntry> rejected = attributes.getIndexRejected();
+ assertTrue(at("indexRejected") + " expected none but was " + rejected,
+ rejected == null || rejected.isEmpty());
+ return this;
+ }
+
+ /**
+ * Assert that the rejected index list contains an entry with {@code name}
and {@code reason}.
+ */
+ public ExplainPlanAssert indexRejectedContains(String name, String reason)
{
+ List<RejectedIndexEntry> rejected = attributes.getIndexRejected();
+ assertNotNull(at("indexRejected") + " must not be null", rejected);
+ for (RejectedIndexEntry entry : rejected) {
+ if (entry.getName().equals(name) && entry.getReason().equals(reason)) {
+ return this;
+ }
+ }
+ throw new AssertionError(at("indexRejected") + " expected to contain {"
+ name + ", " + reason
+ + "} but was " + rejected);
Review Comment:
The AssertionError construction is split, but the first line is still very
long and likely violates the 100-character Checkstyle LineLength rule. Consider
building the message in a local variable with wrapped concatenation.
--
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]
