Copilot commented on code in PR #2497:
URL: https://github.com/apache/phoenix/pull/2497#discussion_r3365328863
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/HashJoinPlan.java:
##########
@@ -313,24 +315,72 @@ private Expression createKeyRangeExpression(Expression
lhsExpression, Expression
@Override
public ExplainPlan getExplainPlan() throws SQLException {
- // TODO : Support ExplainPlanAttributes for HashJoinPlan
- List<String> planSteps =
Lists.newArrayList(delegate.getExplainPlan().getPlanSteps());
+ ExplainPlan delegateExplainPlan = delegate.getExplainPlan();
+ List<String> planSteps =
Lists.newArrayList(delegateExplainPlan.getPlanSteps());
+ // Each hash/skip-scan sub-plan is recorded under subPlans.
+ ExplainPlanAttributes delegateAttributes =
delegateExplainPlan.getPlanStepsAsAttributes();
+ ExplainPlanAttributesBuilder builder = (delegateAttributes != null
+ && delegateAttributes != ExplainPlanAttributes.getDefaultExplainPlan())
+ ? new ExplainPlanAttributesBuilder(delegateAttributes)
+ : null;
Review Comment:
HashJoinPlan#getExplainPlan currently builds an ExplainPlanAttributesBuilder
only when the delegate plan exposes non-default attributes. When the delegate
returns the default ExplainPlanAttributes (e.g., degenerate scans),
HashJoinPlan will drop join-specific structured fields
(subPlans/dynamicServerFilter/afterJoinFilter/joinScannerLimit) even though it
computes them, which undermines the new attribute-based assertions.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/HashJoinPlan.java:
##########
@@ -313,24 +315,72 @@ private Expression createKeyRangeExpression(Expression
lhsExpression, Expression
@Override
public ExplainPlan getExplainPlan() throws SQLException {
- // TODO : Support ExplainPlanAttributes for HashJoinPlan
- List<String> planSteps =
Lists.newArrayList(delegate.getExplainPlan().getPlanSteps());
+ ExplainPlan delegateExplainPlan = delegate.getExplainPlan();
+ List<String> planSteps =
Lists.newArrayList(delegateExplainPlan.getPlanSteps());
+ // Each hash/skip-scan sub-plan is recorded under subPlans.
+ ExplainPlanAttributes delegateAttributes =
delegateExplainPlan.getPlanStepsAsAttributes();
+ ExplainPlanAttributesBuilder builder = (delegateAttributes != null
+ && delegateAttributes != ExplainPlanAttributes.getDefaultExplainPlan())
+ ? new ExplainPlanAttributesBuilder(delegateAttributes)
+ : null;
+ List<ExplainPlanAttributes> subPlanAttributes = Lists.newArrayList();
int count = subPlans.length;
for (int i = 0; i < count; i++) {
planSteps.addAll(subPlans[i].getPreSteps(this));
+ ExplainPlanAttributes subPlanAttribute =
subPlans[i].getPreStepsAsAttributes(this);
+ if (subPlanAttribute != null) {
+ subPlanAttributes.add(subPlanAttribute);
+ }
}
for (int i = 0; i < count; i++) {
- planSteps.addAll(subPlans[i].getPostSteps(this));
+ List<String> postSteps = subPlans[i].getPostSteps(this);
+ planSteps.addAll(postSteps);
+ if (builder != null) {
+ for (String step : postSteps) {
+ String trimmed = step.trim();
+ if (trimmed.startsWith("DYNAMIC SERVER FILTER BY")) {
+ builder.setDynamicServerFilter(trimmed);
+ }
+ }
+ }
}
if (joinInfo != null && joinInfo.getPostJoinFilterExpression() != null) {
- planSteps.add(
- " AFTER-JOIN SERVER FILTER BY " +
joinInfo.getPostJoinFilterExpression().toString());
+ String afterJoinFilter =
+ "AFTER-JOIN SERVER FILTER BY " +
joinInfo.getPostJoinFilterExpression().toString();
+ planSteps.add(" " + afterJoinFilter);
+ if (builder != null) {
+ builder.setAfterJoinFilter(afterJoinFilter);
+ }
}
if (joinInfo != null && joinInfo.getLimit() != null) {
planSteps.add(" JOIN-SCANNER " + joinInfo.getLimit() + " ROW LIMIT");
+ if (builder != null) {
+ builder.setJoinScannerLimit(joinInfo.getLimit().longValue());
+ }
+ }
+ if (builder == null) {
+ return new ExplainPlan(planSteps);
}
- return new ExplainPlan(planSteps);
+ if (!subPlanAttributes.isEmpty()) {
+ builder.setSubPlans(subPlanAttributes);
+ }
+ return new ExplainPlan(planSteps, builder.build());
+ }
+
+ /**
+ * Builds the explain-plan attributes for a hash-join subplan by taking the
inner plan's
+ * attributes and stamping the supplied join header onto
abstractExplainPlan. Returns null when
+ * the inner plan exposes no structured attributes.
+ */
Review Comment:
The Javadoc for subPlanAttributesWithHeader says it "Returns null when the
inner plan exposes no structured attributes", but the implementation always
returns a non-null ExplainPlanAttributes (it falls back to a new builder). This
is misleading for callers and future maintainers.
##########
phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainPlanTestUtil.java:
##########
@@ -0,0 +1,359 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.query.explain;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.List;
+import org.apache.phoenix.compile.ExplainPlan;
+import org.apache.phoenix.compile.ExplainPlanAttributes;
+import org.apache.phoenix.jdbc.PhoenixPreparedStatement;
+
+/**
+ * Test helpers for retrieving the {@link ExplainPlan} and its structured
+ * {@link ExplainPlanAttributes} for a query or mutation (without going
through the textual
+ * {@code EXPLAIN ...} ResultSet path), plus a fluent {@link
ExplainPlanAssert} API for asserting on
+ * the attribute values.
+ */
+public final class ExplainPlanTestUtil {
+
+ private ExplainPlanTestUtil() {
+ }
+
+ /** Optimize {@code query} and return its {@link ExplainPlan}. */
+ public static ExplainPlan getExplainPlan(Connection conn, String query)
throws SQLException {
+ PhoenixPreparedStatement statement =
+ conn.prepareStatement(query).unwrap(PhoenixPreparedStatement.class);
+ return statement.optimizeQuery().getExplainPlan();
+ }
Review Comment:
ExplainPlanTestUtil#getExplainPlan prepares a PhoenixPreparedStatement but
never closes it. In a large refactor like this (dozens of tests), leaving
statements unclosed can accumulate resources and cause test flakiness.
##########
phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainPlanTestUtil.java:
##########
@@ -0,0 +1,359 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.query.explain;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.List;
+import org.apache.phoenix.compile.ExplainPlan;
+import org.apache.phoenix.compile.ExplainPlanAttributes;
+import org.apache.phoenix.jdbc.PhoenixPreparedStatement;
+
+/**
+ * Test helpers for retrieving the {@link ExplainPlan} and its structured
+ * {@link ExplainPlanAttributes} for a query or mutation (without going
through the textual
+ * {@code EXPLAIN ...} ResultSet path), plus a fluent {@link
ExplainPlanAssert} API for asserting on
+ * the attribute values.
+ */
+public final class ExplainPlanTestUtil {
+
+ private ExplainPlanTestUtil() {
+ }
+
+ /** Optimize {@code query} and return its {@link ExplainPlan}. */
+ public static ExplainPlan getExplainPlan(Connection conn, String query)
throws SQLException {
+ PhoenixPreparedStatement statement =
+ conn.prepareStatement(query).unwrap(PhoenixPreparedStatement.class);
+ return statement.optimizeQuery().getExplainPlan();
+ }
+
+ /** Optimize {@code query} and return its structured {@link
ExplainPlanAttributes}. */
+ public static ExplainPlanAttributes getExplainAttributes(Connection conn,
String query)
+ throws SQLException {
+ return getExplainPlan(conn, query).getPlanStepsAsAttributes();
+ }
+
+ /** Optimize {@code query} and return its plan-steps text. */
+ public static List<String> getPlanSteps(Connection conn, String query)
throws SQLException {
+ return getExplainPlan(conn, query).getPlanSteps();
+ }
+
+ /** Compile a mutation (UPSERT/DELETE) and return its {@link ExplainPlan}. */
+ public static ExplainPlan getMutationExplainPlan(Connection conn, String
query)
+ throws SQLException {
+ PhoenixPreparedStatement statement =
+ conn.prepareStatement(query).unwrap(PhoenixPreparedStatement.class);
+ return statement.compileMutation().getExplainPlan();
+ }
Review Comment:
ExplainPlanTestUtil#getMutationExplainPlan also prepares a
PhoenixPreparedStatement without closing it. This has the same resource-leak
risk as getExplainPlan().
--
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]
