Skip to content

Commit 7de6215

Browse files
committed
Add containsSubQuery fast-path + unit tests on the discriminator
Without the fast-path, the rewrite path's HepPlanner.setRoot(plan) calls .getCluster() on the input — which is null on the mocked RelNodes the existing AnalyticsExecutionEngineTest passes through execute(). Six of the suite's nine tests started failing with 'NullPointerException: cluster' after this PR's first commit. Add a containsSubQuery(RelNode) pre-check that walks the plan with a RelHomogeneousShuttle + RexShuttle, returns true the moment it sees a RexSubQuery, and false otherwise. The rewrite chain only runs when the discriminator says there's something to remove — restores compatibility with the existing mock-based tests and skips HEP entirely for the typical query that has no subqueries. Adds two focused unit tests on the discriminator (package-private so the test can call it directly): * containsSubQuery_falseForPlanWithoutSubQuery — vanilla mocked plan reports false, preventing the HEP/decorrelator path from ever firing on cluster-less mocks. * containsSubQuery_trueWhenRelNodeExposesRexSubQueryDuringTraversal — stub the RelNode.accept(RexShuttle) to surface a RexSubQuery and verify it's detected. The HEP+decorrelator chain itself is Calcite-tested; the end-to-end behavior is covered by CalcitePPLExistsSubqueryIT (0/19 -> 6/19 pass, zero remaining 'Unrecognized filter operator' failures). Signed-off-by: Jialiang Liang <jialianl@amazon.com> Signed-off-by: Jialiang Liang <jiallian@amazon.com>
1 parent 328ee1b commit 7de6215

2 files changed

Lines changed: 85 additions & 0 deletions

File tree

core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngine.java

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,13 @@
1313
import org.apache.calcite.plan.hep.HepPlanner;
1414
import org.apache.calcite.plan.hep.HepProgram;
1515
import org.apache.calcite.plan.hep.HepProgramBuilder;
16+
import org.apache.calcite.rel.RelHomogeneousShuttle;
1617
import org.apache.calcite.rel.RelNode;
1718
import org.apache.calcite.rel.rules.CoreRules;
1819
import org.apache.calcite.rel.type.RelDataType;
1920
import org.apache.calcite.rel.type.RelDataTypeField;
21+
import org.apache.calcite.rex.RexShuttle;
22+
import org.apache.calcite.rex.RexSubQuery;
2023
import org.apache.calcite.sql2rel.RelDecorrelator;
2124
import org.opensearch.analytics.exec.QueryPlanExecutor;
2225
import org.opensearch.core.action.ActionListener;
@@ -177,9 +180,38 @@ private ExprType convertType(RelDataType type) {
177180
.build();
178181

179182
private static RelNode removeSubQueries(RelNode plan, CalcitePlanContext context) {
183+
// Fast-path: skip HEP + decorrelation entirely if the plan has no RexSubQuery. Avoids HEP
184+
// overhead for the typical query and keeps the rewrite a no-op for callers (incl. unit tests)
185+
// whose RelNodes don't have a real RelOptCluster wired up.
186+
if (!containsSubQuery(plan)) {
187+
return plan;
188+
}
180189
HepPlanner planner = new HepPlanner(SUBQUERY_REMOVE_PROGRAM);
181190
planner.setRoot(plan);
182191
RelNode withCorrelates = planner.findBestExp();
183192
return RelDecorrelator.decorrelateQuery(withCorrelates, context.relBuilder);
184193
}
194+
195+
/** Walks {@code plan} and returns {@code true} as soon as any {@link RexSubQuery} is found. */
196+
static boolean containsSubQuery(RelNode plan) {
197+
boolean[] found = {false};
198+
RexShuttle rexFinder =
199+
new RexShuttle() {
200+
@Override
201+
public org.apache.calcite.rex.RexNode visitSubQuery(RexSubQuery sub) {
202+
found[0] = true;
203+
return sub;
204+
}
205+
};
206+
plan.accept(
207+
new RelHomogeneousShuttle() {
208+
@Override
209+
public RelNode visit(RelNode node) {
210+
if (found[0]) return node;
211+
node.accept(rexFinder);
212+
return found[0] ? node : super.visit(node);
213+
}
214+
});
215+
return found[0];
216+
}
185217
}

core/src/test/java/org/opensearch/sql/executor/analytics/AnalyticsExecutionEngineTest.java

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package org.opensearch.sql.executor.analytics;
77

88
import static org.junit.jupiter.api.Assertions.assertEquals;
9+
import static org.junit.jupiter.api.Assertions.assertFalse;
910
import static org.junit.jupiter.api.Assertions.assertNotNull;
1011
import static org.junit.jupiter.api.Assertions.assertTrue;
1112
import static org.mockito.ArgumentMatchers.any;
@@ -20,9 +21,12 @@
2021
import java.util.concurrent.atomic.AtomicReference;
2122
import java.util.stream.Collectors;
2223
import org.apache.calcite.rel.RelNode;
24+
import org.apache.calcite.rel.RelShuttle;
2325
import org.apache.calcite.rel.type.RelDataType;
2426
import org.apache.calcite.rel.type.RelDataTypeFactory;
2527
import org.apache.calcite.rel.type.RelDataTypeSystem;
28+
import org.apache.calcite.rex.RexShuttle;
29+
import org.apache.calcite.rex.RexSubQuery;
2630
import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
2731
import org.apache.calcite.sql.type.SqlTypeName;
2832
import org.junit.jupiter.api.BeforeEach;
@@ -276,6 +280,55 @@ void physicalPlanExplain_callsOnFailure() {
276280
+ errorRef.get().getMessage());
277281
}
278282

283+
// --- Subquery-removal discriminator (see {@link AnalyticsExecutionEngine#containsSubQuery}) ---
284+
//
285+
// {@code containsSubQuery} gates the SubQueryRemoveRule + RelDecorrelator chain we run inside
286+
// {@code execute()} / {@code explain()}. The chain is Calcite-tested, but the *discriminator*
287+
// is plugin code; if it falsely returns true for a vanilla mock RelNode the existing
288+
// execute-path tests in this class break with "NullPointerException: cluster" inside HepPlanner.
289+
// If it falsely returns false on a plan that does contain a RexSubQuery, the EXISTS-subquery
290+
// regression we shipped this fix for would silently reappear.
291+
292+
@Test
293+
void containsSubQuery_falseForPlanWithoutSubQuery() {
294+
// A plain mocked RelNode has no RexNodes — Mockito default returns null from accept(...),
295+
// which keeps the shuttle from ever reaching visitSubQuery.
296+
RelNode plan = mock(RelNode.class);
297+
298+
assertFalse(
299+
AnalyticsExecutionEngine.containsSubQuery(plan),
300+
"containsSubQuery should report false on a plan with no RexSubQuery — otherwise the"
301+
+ " HepPlanner / RelDecorrelator path would fire on plans that have no subqueries to"
302+
+ " remove (and on tests that pass mocked RelNodes without a real cluster).");
303+
}
304+
305+
@Test
306+
void containsSubQuery_trueWhenRelNodeExposesRexSubQueryDuringTraversal() {
307+
// Stub the RelNode.accept(RexShuttle) call to feed a RexSubQuery into the shuttle, mirroring
308+
// what a real LogicalFilter(condition=[EXISTS(subq)]) would do during traversal.
309+
RelNode plan = mock(RelNode.class);
310+
RexSubQuery subQuery = mock(RexSubQuery.class);
311+
when(plan.accept(any(RelShuttle.class)))
312+
.thenAnswer(
313+
inv -> {
314+
RelShuttle shuttle = inv.getArgument(0);
315+
return shuttle.visit(plan);
316+
});
317+
when(plan.accept(any(RexShuttle.class)))
318+
.thenAnswer(
319+
inv -> {
320+
RexShuttle rex = inv.getArgument(0);
321+
rex.visitSubQuery(subQuery);
322+
return null;
323+
});
324+
325+
assertTrue(
326+
AnalyticsExecutionEngine.containsSubQuery(plan),
327+
"containsSubQuery should detect a RexSubQuery surfaced through the node's"
328+
+ " accept(RexShuttle) — the discriminator is what gates the EXISTS/IN/SOME/ANY"
329+
+ " removal pass.");
330+
}
331+
279332
// --- helpers ---
280333

281334
private QueryResponse executeAndCapture(RelNode relNode) {

0 commit comments

Comments
 (0)