Copilot commented on code in PR #2545:
URL: https://github.com/apache/phoenix/pull/2545#discussion_r3471213164
##########
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ExplainTable.java:
##########
@@ -620,10 +637,38 @@ private void emitProject(List<String> planSteps,
}
columns.add(name);
}
- planSteps.add(" PROJECT " + String.join(", ", columns));
- if (explainPlanAttributesBuilder != null) {
- explainPlanAttributesBuilder.setServerProject(columns);
+ return columns;
+ }
+
+ /**
+ * Server-side {@code UPSERT SELECT} and {@code DELETE} inner plans are
compiled as an aggregating
+ * {@code SELECT COUNT(1)} whose count is used solely to report how many
rows were touched. That
+ * count is an internal compiler choice. Surfacing {@code PROJECT COUNT(1)}
on the explain output
+ * is misleading because the user asked to upsert/delete a set of
columns/rows, not to count them.
+ * @param planSteps the composed mutation plan steps (mutated in place)
+ * @param innerAttributes the inner aggregate plan's rendered attributes
(source of the count-form
+ * {@code serverProject})
+ * @param builder the mutation plan's attribute builder (its {@code
serverProject} is
+ * overwritten)
+ * @param userProjector the user-facing projection to surface instead of
the count form
+ */
+ public static void overrideMutationProject(List<String> planSteps,
+ ExplainPlanAttributes innerAttributes, ExplainPlanAttributesBuilder
builder,
+ RowProjector userProjector) {
+ List<String> countProject = innerAttributes.getServerProject();
+ if (countProject == null || countProject.isEmpty()) {
+ return;
+ }
Review Comment:
overrideMutationProject assumes planSteps, innerAttributes, and builder are
non-null and also overwrites any non-empty serverProject, even if it isn't the
COUNT(*)/COUNT(1) internal aggregate. Since this is a public static helper, a
null innerAttributes would NPE, and overriding a non-count projection could
produce incorrect/misleading EXPLAIN output if the inner plan changes in the
future. Add defensive null checks and only apply the rewrite when the inner
serverProject clearly looks like a COUNT aggregate.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/compile/DeleteCompiler.java:
##########
@@ -1002,6 +1002,10 @@ public ExplainPlan getExplainPlan() throws SQLException {
planSteps.add(" RETURNING *");
}
planSteps.addAll(queryPlanSteps);
+ // Surface the row-identity projection the scan actually reads so
VERBOSE explain describes
+ // the delete rather than the count.
+ ExplainTable.overrideMutationProject(planSteps, explainPlanAttributes,
newBuilder,
+ dataPlan.getProjector());
Review Comment:
The new explain rewrite for server-side DELETE changes VERBOSE-only plan
output/JSON (PROJECT line + serverProject) but there is no test asserting the
mutation explain under VERBOSE no longer exposes COUNT(1). Add a test that runs
the delete with ExplainOptions.VERBOSE (or EXPLAIN (VERBOSE) ...) and asserts
the rewritten PROJECT/serverProject reflects the row-identity projection rather
than COUNT(1).
##########
phoenix-core-client/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java:
##########
@@ -1261,6 +1261,9 @@ public ExplainPlan getExplainPlan() throws SQLException {
planSteps.add(" RETURNING *");
}
planSteps.addAll(queryPlanSteps);
+ // Surface the user's SELECT projection instead so VERBOSE explain
describes the upsert.
+ ExplainTable.overrideMutationProject(planSteps, explainPlanAttributes,
newBuilder,
+ queryPlan.getProjector());
Review Comment:
The new explain rewrite for server-side UPSERT SELECT changes VERBOSE-only
plan output/JSON (PROJECT line + serverProject) but there is no test asserting
the mutation explain under VERBOSE no longer exposes COUNT(1). Consider adding
an end-to-end explain test that exercises the mutation with
ExplainOptions.VERBOSE (or via EXPLAIN (VERBOSE) ...), and asserts
serverProject/PROJECT matches the user's SELECT projection.
--
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]
