Extend V2 SQL parser with `IN`/`EXISTS` subqueries for unified query path by dai-chen · Pull Request #5448 · opensearch-project/sql

dai-chen · 2026-05-15T21:47:35Z

Description

Extend the V2 SQL ANTLR grammar and add ExtendedAstExpressionBuilder to support IN and EXISTS subqueries in the unified Calcite-based query path. The base AstExpressionBuilder continues to throw SyntaxCheckException for these statements, preserving the legacy engine fallback in production.

Notes

Fix Alias handling in CalciteRelNodeVisitor here to unblock tests using SELECT with specific fields
EXISTS subquery only works for PartiQL nested fields in the legacy engine, thus no IT added here

Planned PRs

PR 1: Route unified SQL path through V2 ANTLR parser #5438
PR 2: Register missing SQL V2 relevance functions in Calcite function table #5440
PR 3a: Extend V2 SQL parser with JOIN for unified query path #5446
PR 3b (current): Extend V2 grammar/parser for IN and EXISTS subqueries
PR 4: Fix remaining CalciteRelNodeVisitor issues identified during V2 AST integration

Related Issues

Part of #5248

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-05-15T21:48:53Z

PR Reviewer Guide 🔍

(Review updated until commit `bd3074d`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Fix The Alias handling fix only adds the alias to expandedFields but does not add it to addedFields. This means if the same alias appears multiple times in the project list, it will be added to expandedFields repeatedly instead of being deduplicated. The fix should include 'addedFields.add(alias.getName())' or similar logic to track that this alias has been processed. case Alias alias -> { expandedFields.add(rexVisitor.analyze(alias, context)); } Initialization Order The constructor calls 'createExpressionBuilder()' before the 'query' field is fully initialized in the superclass chain. If a subclass overrides 'createExpressionBuilder()' and that override accesses 'this.query', it will see null. This can cause NullPointerException if the expression builder needs the query string during construction. public AstBuilder(String query) { this.query = query; this.expressionBuilder = createExpressionBuilder(); }

github-actions · 2026-05-15T21:49:17Z

PR Code Suggestions ✨

Latest suggestions up to bd3074d

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Verify IN subquery value mapping The `visit(ctx.predicate())` call processes the left-hand side expression, but `inSubquery` expects values as varargs. If `ctx.predicate()` returns a complex expression or multiple values, this may not correctly represent the IN clause semantics. Verify that the predicate context correctly maps to the expected value list for the IN operation. api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java [92-96] @Override public UnresolvedExpression visitInSubqueryPredicate(InSubqueryPredicateContext ctx) { UnresolvedPlan subquery = ExtendedAstBuilder.this.visit(ctx.querySpecification()); - UnresolvedExpression inExpr = inSubquery(subquery, visit(ctx.predicate())); + UnresolvedExpression leftExpr = visit(ctx.predicate()); + UnresolvedExpression inExpr = inSubquery(subquery, leftExpr); return (ctx.NOT() != null) ? new Not(inExpr) : inExpr; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential semantic issue where `visit(ctx.predicate())` processes the left-hand side expression for the IN clause. The concern about whether this correctly maps to the expected value list is valid and worth verification, though the improved code doesn't substantially change the logic beyond adding an intermediate variable for clarity.	Medium
General	Prevent duplicate alias field expansion Adding `Alias` handling in `expandProjectFields` may cause duplicate field expansion if aliases are already processed elsewhere in the projection logic. Ensure that this doesn't conflict with existing alias resolution or cause fields to be added multiple times to `expandedFields`. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [544-546] case Alias alias -> { - expandedFields.add(rexVisitor.analyze(alias, context)); + if (addedFields.add(alias.getName())) { + expandedFields.add(rexVisitor.analyze(alias, context)); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about potential duplicate field expansion when handling `Alias` cases. Adding a check using `addedFields.add(alias.getName())` before adding to `expandedFields` could prevent duplicates, though it's unclear if this is actually a problem in the current implementation without more context about how aliases are processed elsewhere.	Low

Previous suggestions

Suggestions up to commit 62a146b

Category	Suggestion	Impact
Possible issue	Handle NOT IN subquery case The `visitInSubqueryPredicate` method doesn't handle the `NOT IN` case. The grammar allows `NOT? IN`, but the implementation always creates a positive `IN` subquery. Check `ctx.NOT()` and negate the result accordingly to support `NOT IN` subqueries. api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java [90-94] -private class ExtendedAstExpressionBuilder extends AstExpressionBuilder { +@Override +public UnresolvedExpression visitInSubqueryPredicate(InSubqueryPredicateContext ctx) { + UnresolvedPlan subquery = ExtendedAstBuilder.this.visit(ctx.querySpecification()); + UnresolvedExpression inExpr = inSubquery(subquery, visit(ctx.predicate())); + return ctx.NOT() != null ? not(inExpr) : inExpr; +} - @Override - public UnresolvedExpression visitInSubqueryPredicate(InSubqueryPredicateContext ctx) { - UnresolvedPlan subquery = ExtendedAstBuilder.this.visit(ctx.querySpecification()); - return inSubquery(subquery, visit(ctx.predicate())); - } - Suggestion importance[1-10]: 8 __ Why: This is a critical correctness issue. The grammar allows `NOT? IN` for subqueries, but the implementation doesn't handle the `NOT` case, which would cause incorrect query results when `NOT IN` is used with subqueries.	Medium

Suggestions up to commit cc181cd

Category	Suggestion	Impact
General	Track alias in added fields Adding `Alias` handling in the project expansion logic may cause duplicate field names or incorrect field references if the alias is not properly tracked in `addedFields`. Consider adding the alias name to `addedFields` to prevent duplicates and ensure consistent field tracking across the expansion logic. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [544-546] case Alias alias -> { + addedFields.add(alias.getName()); expandedFields.add(rexVisitor.analyze(alias, context)); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential issue where `Alias` fields might not be tracked in `addedFields`, which could lead to duplicates or inconsistent field tracking. Adding `alias.getName()` to `addedFields` would maintain consistency with how other field types are handled in the same method.	Low
General	Verify single predicate wrapping The `InSubquery` constructor expects a list of expressions, but only a single predicate is being passed. This may cause issues if the IN clause should support multiple values on the left side (e.g., `(col1, col2) IN (SELECT ...)`). Verify that wrapping a single predicate in a list is the intended behavior for all IN subquery cases. api/src/main/java/org/opensearch/sql/api/parser/SqlV2QueryParser.java [88-91] public UnresolvedExpression visitInSubqueryPredicate(InSubqueryPredicateContext ctx) { UnresolvedPlan subquery = ExtendedAstBuilder.this.visit(ctx.querySpecification()); - return new InSubquery(List.of(visit(ctx.predicate())), subquery); + UnresolvedExpression predicate = visit(ctx.predicate()); + return new InSubquery(List.of(predicate), subquery); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that a single predicate is wrapped in a list for `InSubquery`. However, the 'improved_code' is functionally identical to the 'existing_code', only extracting the predicate to a variable. The concern about multi-column IN clauses is valid but requires verification rather than a code change, limiting the impact.	Low

github-actions · 2026-05-15T22:21:07Z

Persistent review updated to latest commit 62a146b

Add grammar rules (inSubqueryPredicate, existsSubqueryExpressionAtom) and wire them through ExtendedAstExpressionBuilder to produce InSubquery and ExistsSubquery AST nodes for the Calcite-based unified query path. Base AstExpressionBuilder throws SyntaxCheckException to preserve legacy engine fallback. AstBuilder now uses createExpressionBuilder() factory method to allow subclass customization. Also add Alias handling in CalciteRelNodeVisitor.expandProjectFields required for any non-SELECT * query in the unified path. Signed-off-by: Chen Dai <daichen@amazon.com>

github-actions · 2026-05-16T04:46:37Z

Persistent review updated to latest commit bd3074d

Swiddis · 2026-05-18T17:14:18Z

+  public UnresolvedExpression visitExistsSubqueryExpressionAtom(
+      ExistsSubqueryExpressionAtomContext ctx) {
+    throw new SyntaxCheckException(
+        "EXISTS subquery is not supported in the V2 SQL engine. Falling back to legacy engine.");


suggestion: Change these to NotSupportedException or similar to make downstream handling more precise.

It's not technically a syntax error to use an unsupported operation and I think it'd be good to distinguish "the engine can't handle your query" from "your query is bad."

Might need to make a new exception type if we don't have one. If we use the design from #5448 (comment), a clearer option might be NotEnabledException.

yeah, it make senses but that introduces many changes for this temporary workaround. Probably we can defer this refactor?

Swiddis · 2026-05-18T17:16:13Z

+  public UnresolvedExpression visitExistsSubqueryExpressionAtom(
+      ExistsSubqueryExpressionAtomContext ctx) {
+    throw new SyntaxCheckException(
+        "EXISTS subquery is not supported in the V2 SQL engine. Falling back to legacy engine.");


suggestion: I would prefer we don't explicitly state "Falling back to legacy engine" in the exception since that's really the exception handler's concern

I thought this is internal and won't be returned. Let me double check. Thanks!

Swiddis · 2026-05-18T17:24:08Z

+     * visit subquery plan nodes. Must be created via {@link #createExpressionBuilder()} because the
+     * enclosing {@code this} reference is not available during {@code super()} construction.
+     */
+    private class ExtendedAstExpressionBuilder extends AstExpressionBuilder {


issue: If this is only relevant to legacy we should call this LegacyExpressionBuilder.

We hope that eventually the primary expression builder will have more features than legacy. I also suspect without proof that if we start splitting out the builder, we'll eventually have yet another builder with a different set of features.

V2's expression builder is AstExpressionBuilder so this one is ExtendedAstExpressionBuilder which extends V2's?

I was going off the exceptions saying 'falling back to legacy'

Execution path 1: V2 grammar/AstBuilder + Legacy (will be migrating to path 2)

Execution path 2: V2 extended grammar/AstBuilder + CalciteRelNodeVisitor

Swiddis · 2026-05-18T17:25:07Z

+     * visit subquery plan nodes. Must be created via {@link #createExpressionBuilder()} because the
+     * enclosing {@code this} reference is not available during {@code super()} construction.
+     */
+    private class ExtendedAstExpressionBuilder extends AstExpressionBuilder {


suggestion (architecture): I would prefer if we put these additional predicates behind configured flags instead of making a whole new builder for them.

This makes it easier to find where AST visit code is defined since it's all in the same place, and makes it easy to see at a glance under what conditions different features are enabled. It also means that we can easily have multiple types of builders share functionality: if we eventually need another subclass of AstExpressionBuilder we may end up needing to duplicate visit code too.

One way might be to have AstExpressionBuilder take a enableSubqueryPredicates flag as part of its construction (or enableExtendedFeatures, or enableLegacyFeatures...). Another could be supplying an Engine enum and using that.

To give you some context: this design is a temporary workaround to enable SQL V2 + Calcite on the Analytics Engine path with minimal impact on existing SQL. Given that, I'd like to keep the change surface as small as possible and avoid introducing a flag-based mechanism.

penghuo · 2026-05-18T17:46:43Z

+        .assertPlan(
+            """
+            LogicalProject(name=[$1])
+              LogicalFilter(condition=[IN($2, {


Does this logical plan can be rewrite as Join? PPL has similar execution path?

Yes, confirmed that PPL generates similar RelNode plan: https://github.com/opensearch-project/sql/blob/main/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLInSubqueryTest.java#L38

PR opensearch-project#5448 (parent commit b10d541) introduced SQL IN/EXISTS subquery support that produces a Project AST whose project list contains Alias expressions. The 'Lower structured search predicates' commit (c18b11e) on this PR accidentally dropped the case Alias branch from CalciteRelNodeVisitor.expandProjectFields, so the subquery's project list hit the default arm and threw 'Unexpected expression type in project list: Alias'. Restoring the one-line branch fixes UnifiedQueryPlannerSqlV2Test testInSubquery / testNotInSubquery / testExistsSubquery / testNotExistsSubquery (the four macOS unit-test failures on this PR). Signed-off-by: Kai Huang <ahkcs@amazon.com>

…search-project#5448) Add grammar rules (inSubqueryPredicate, existsSubqueryExpressionAtom) and wire them through ExtendedAstExpressionBuilder to produce InSubquery and ExistsSubquery AST nodes for the Calcite-based unified query path. Base AstExpressionBuilder throws SyntaxCheckException to preserve legacy engine fallback. AstBuilder now uses createExpressionBuilder() factory method to allow subclass customization. Also add Alias handling in CalciteRelNodeVisitor.expandProjectFields required for any non-SELECT * query in the unified path. Signed-off-by: Chen Dai <daichen@amazon.com>

dai-chen self-assigned this May 15, 2026

dai-chen added enhancement New feature or request SQL labels May 15, 2026

dai-chen force-pushed the feature/sql-v2-subquery branch from cc181cd to 62a146b Compare May 15, 2026 22:19

dai-chen marked this pull request as ready for review May 16, 2026 00:02

dai-chen requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners May 16, 2026 00:02

dai-chen force-pushed the feature/sql-v2-subquery branch from 62a146b to bd3074d Compare May 16, 2026 04:45

Swiddis reviewed May 18, 2026

View reviewed changes

penghuo reviewed May 18, 2026

View reviewed changes

penghuo approved these changes May 18, 2026

View reviewed changes

Swiddis approved these changes May 18, 2026

View reviewed changes

dai-chen merged commit b10d541 into opensearch-project:main May 18, 2026
38 of 40 checks passed

Conversation

dai-chen commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

Planned PRs

Related Issues

Check List

Uh oh!

github-actions Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit bd3074d)

Uh oh!

github-actions Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

github-actions Bot commented May 16, 2026

Uh oh!

Swiddis May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiddis May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dai-chen May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiddis May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dai-chen commented May 15, 2026 •

edited

Loading

github-actions Bot commented May 15, 2026 •

edited

Loading

(Review updated until commit `bd3074d`)

github-actions Bot commented May 15, 2026 •

edited

Loading

Swiddis May 18, 2026 •

edited

Loading

Swiddis May 18, 2026 •

edited

Loading

dai-chen May 18, 2026 •

edited

Loading

Swiddis May 18, 2026 •

edited

Loading