Add Calcite native SQL planning in UnifiedQueryPlanner

[dai-chen]

Description

Adds SQL support to the unified query API using Calcite's native parser pipeline, enabling ANSI SQL queries to produce RelNode logical plans alongside the existing PPL path. Specifically, SQL planning path: SqlParser → SqlValidator → SqlToRelConverter → RelNode.

Next Steps

• Extract UnifiedQueryParser to decouple parsing from planning
• Breaking change analysis comparing Calcite SQL behavior with SQL V2

Related Issues

Resolves #5248 (subtask 1)

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]

PR Reviewer Guide 🔍

(Review updated until commit dbde012)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [52]: Generate optimal physical plan to fully unify query execution and leverage Calcite's optimizer.

🔀 Multiple PR themes Sub-PR theme: Add Calcite native SQL planning strategy to UnifiedQueryPlanner Relevant files: api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java api/src/test/java/org/opensearch/sql/api/UnifiedSqlQueryPlannerTest.java api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java api/src/testFixtures/java/org/opensearch/sql/api/UnifiedQueryTestBase.java api/README.md Sub-PR theme: Extend benchmark to cover SQL query patterns alongside PPL Relevant files: benchmarks/src/jmh/java/org/opensearch/sql/api/UnifiedQueryBenchmark.java

⚡ Recommended focus areas for review Resource Leak CalciteSqlStrategy.plan() opens a Planner via Frameworks.getPlanner(...) inside a try-with-resources block, which is correct. However, the Planner is a stateful, non-thread-safe object that is created fresh on every plan() call. If planner.parse() or planner.validate() throws an exception that is not Exception (e.g., an Error), the try-with-resources still handles it. This is fine. But note that Planner implements AutoCloseable and the try-with-resources is correct here — no actual leak. This is a non-issue on closer inspection. try (Planner planner = Frameworks.getPlanner(context.getPlanContext().config)) { SqlNode parsed = planner.parse(query); SqlNode validated = planner.validate(parsed); RelRoot relRoot = planner.rel(validated); return relRoot.rel; } } Incomplete RelRoot CalciteSqlStrategy.plan() returns relRoot.rel directly, discarding relRoot.collation, relRoot.fields, and relRoot.kind. For queries with ORDER BY, Calcite may encode the sort in relRoot.collation rather than in relRoot.rel itself, meaning the ordering could be silently dropped. The AstStrategy explicitly handles this via preserveCollation, but CalciteSqlStrategy does not apply equivalent logic. Consider using relRoot.project() or checking relRoot.isRefTrivial() before returning relRoot.rel. RelRoot relRoot = planner.rel(validated); return relRoot.rel; Null Query query is set by (language.equals("PPL") ? PPL_QUERIES : SQL_QUERIES).get(queryPattern). If queryPattern is ever a value not present in the map (e.g., due to a typo or future param addition), query will be null, causing a NullPointerException at benchmark runtime with no clear error message. A null-check or getOrDefault with a descriptive error would make failures easier to diagnose. query = (language.equals("PPL") ? PPL_QUERIES : SQL_QUERIES).get(queryPattern);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dbde012

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use project() to preserve ORDER BY collation relRoot.rel returns only the top-level relational node without applying any collation or trimming. For queries with ORDER BY, the sort information is stored in relRoot but may not be reflected in relRoot.rel alone. Consider using relRoot.project() instead, which properly handles the top-level projection and collation. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [79-86] @Override public RelNode plan(String query) throws Exception { try (Planner planner = Frameworks.getPlanner(context.getPlanContext().config)) { SqlNode parsed = planner.parse(query); SqlNode validated = planner.validate(parsed); RelRoot relRoot = planner.rel(validated); - return relRoot.rel; + return relRoot.project(); } } Suggestion importance[1-10]: 6 __ Why: Using relRoot.project() instead of relRoot.rel is a valid concern for SQL queries with ORDER BY, as relRoot.rel may not include the sort node. The testSelectWithOrderBy test only checks assertNotNull, so this bug could go undetected. The suggestion is accurate and addresses a real correctness issue.	Low
General	Preserve original error message in wrapped exception The SyntaxCheckException is only thrown by the AstStrategy (PPL path), not by CalciteSqlStrategy. SQL parsing errors from Calcite will be wrapped in IllegalStateException, losing the original error type and message clarity. Consider catching Calcite-specific parse/validation exceptions and rethrowing them as a more informative exception type, or at minimum preserving the original cause in the IllegalStateException. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [58-66] public RelNode plan(String query) { try { return strategy.plan(query); } catch (SyntaxCheckException e) { throw e; } catch (Exception e) { - throw new IllegalStateException("Failed to plan query", e); + throw new IllegalStateException("Failed to plan query: " + e.getMessage(), e); } } Suggestion importance[1-10]: 3 __ Why: The original IllegalStateException already preserves the cause via e, so the message is accessible. Adding e.getMessage() to the string is a minor improvement but the cause is already not lost. The improved_code accurately reflects the change but the impact is minimal.	Low
	Verify exception cause is preserved in test The test only verifies the exception type but not the message or cause. A more robust test would also verify that the exception message contains meaningful information about the failure, ensuring the error wrapping in plan() works correctly. api/src/test/java/org/opensearch/sql/api/UnifiedSqlQueryPlannerTest.java [179-182] @Test public void testInvalidSqlThrowsException() { - assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM")); + IllegalStateException ex = + assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM")); + assertNotNull("Exception should have a cause", ex.getCause()); } Suggestion importance[1-10]: 3 __ Why: Adding a cause assertion improves test robustness, but this is a minor test quality improvement. The improved_code accurately reflects the change, though the impact on correctness is low.	Low

---

Previous suggestions

Suggestions up to commit 2cd1e28

Category	Suggestion	Impact
General	Avoid recreating planner on every query call The SQLStrategy.plan method uses a try-with-resources block that closes the Planner after each call. However, Frameworks.getPlanner creates a new planner instance on every plan() invocation, which is expensive. The Planner should be created once and reused (or reset between calls using planner.close() and re-initialization), rather than being recreated for every query. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [79-86] -public RelNode plan(String query) { - try { - return strategy.plan(query); - } catch (SyntaxCheckException e) { - throw e; - } catch (Exception e) { - throw new IllegalStateException("Failed to plan query", e); +@RequiredArgsConstructor +private static class SQLStrategy implements PlanningStrategy { + private final UnifiedQueryContext context; + private final Planner planner; + + SQLStrategy(UnifiedQueryContext context) { + this.context = context; + this.planner = Frameworks.getPlanner(context.getPlanContext().config); + } + + @Override + public RelNode plan(String query) throws Exception { + planner.close(); // reset state for reuse + SqlNode parsed = planner.parse(query); + SqlNode validated = planner.validate(parsed); + RelRoot relRoot = planner.rel(validated); + return relRoot.rel; } } Suggestion importance[1-10]: 6 __ Why: Creating a new Planner instance on every plan() call via Frameworks.getPlanner is indeed expensive. Reusing the planner with planner.close() to reset state between calls is a valid performance improvement. However, the improved_code has a constructor issue with @RequiredArgsConstructor and a manual constructor coexisting, making it slightly inaccurate.	Low
	Improve invalid SQL exception test coverage The test testInvalidSqlThrowsException only checks that an IllegalStateException is thrown for invalid SQL, but a syntax error should ideally throw a SyntaxCheckException (which is re-thrown unwrapped in plan()). Consider also testing that truly malformed SQL (not just incomplete SQL) throws the appropriate exception type, and verify the exception message contains useful information. api/src/test/java/org/opensearch/sql/api/UnifiedSqlQueryPlannerTest.java [179-182] @Test public void testInvalidSqlThrowsException() { - assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM")); + IllegalStateException ex = + assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM")); + assertNotNull("Exception cause should be present", ex.getCause()); } +@Test +public void testCompletelyInvalidSqlThrowsException() { + assertThrows(Exception.class, () -> planner.plan("NOT VALID SQL AT ALL !!!")); +} + Suggestion importance[1-10]: 3 __ Why: The suggestion to verify the exception cause and add an additional test case is a minor improvement to test coverage. The existing test already validates the exception type, and the additional coverage is marginal.	Low
Possible issue	Add null-check for missing benchmark query patterns If queryPattern does not match any key in PPL_QUERIES or SQL_QUERIES, query will be null, causing a NullPointerException during the benchmark run with no clear error message. Add a null-check or validation after the map lookup to fail fast with a descriptive error. benchmarks/src/jmh/java/org/opensearch/sql/api/UnifiedQueryBenchmark.java [102-108] @Setup(Level.Trial) public void setUpBenchmark() { super.setUp(); query = (language.equals("PPL") ? PPL_QUERIES : SQL_QUERIES).get(queryPattern); + if (query == null) { + throw new IllegalStateException( + "No query found for language=" + language + ", pattern=" + queryPattern); + } transpiler = UnifiedQueryTranspiler.builder().dialect(SparkSqlDialect.DEFAULT).build(); compiler = new UnifiedQueryCompiler(context); } Suggestion importance[1-10]: 5 __ Why: Since @Param values are fixed and match the map keys exactly, a null result is unlikely in practice, but the defensive null-check is still a reasonable safeguard that would provide a clearer error message if the parameters ever diverge from the map keys.	Low

Suggestions up to commit 6bf098d

Category	Suggestion	Impact
Possible issue	Preserve ORDER BY collation in SQL plans RelRoot.rel returns only the top-level relational expression but discards the collation and other root-level metadata. For queries with ORDER BY, the sort information is captured in RelRoot and may not be present in relRoot.rel alone. Use relRoot.project() or apply the same preserveCollation logic used in PPLStrategy to ensure ordering is preserved for SQL queries too. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [86-94] @Override public RelNode plan(String query) throws Exception { try (Planner planner = Frameworks.getPlanner(context.getPlanContext().config)) { SqlNode parsed = planner.parse(query); SqlNode validated = planner.validate(parsed); RelRoot relRoot = planner.rel(validated); - return relRoot.rel; + return relRoot.project(); } } Suggestion importance[1-10]: 7 __ Why: Using relRoot.rel discards ORDER BY collation for SQL queries, while PPLStrategy explicitly handles collation via preserveCollation. Using relRoot.project() is the standard Calcite approach to preserve sort information, making this a meaningful correctness fix.	Medium
	Map SQL parse/validation errors to syntax exceptions The SyntaxCheckException catch block re-throws without wrapping, but SQL planning via Calcite's Planner will throw Calcite-specific exceptions (e.g., SqlParseException, ValidationException) that are not SyntaxCheckException instances. These will be caught by the generic Exception handler and wrapped in IllegalStateException, losing the semantic distinction between syntax errors and other failures. Consider mapping Calcite SQL parse/validation exceptions to SyntaxCheckException as well. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [68-72] } catch (SyntaxCheckException e) { throw e; + } catch (org.apache.calcite.tools.ValidationException | org.apache.calcite.sql.parser.SqlParseException e) { + throw new SyntaxCheckException(e.getMessage(), e); } catch (Exception e) { throw new IllegalStateException("Failed to plan query", e); } Suggestion importance[1-10]: 5 __ Why: The suggestion is valid — Calcite SQL exceptions won't be caught as SyntaxCheckException, losing semantic distinction. However, this is a minor improvement since the IllegalStateException wrapping still preserves the original cause, and the PR is in an experimental API stage.	Low
General	Guard against missing query pattern keys If queryPattern does not exist as a key in the map, get() returns null silently, causing a NullPointerException later during benchmarking with no clear error message. Add a null-check or use getOrDefault with a meaningful fallback or exception. benchmarks/src/jmh/java/org/opensearch/sql/api/UnifiedQueryBenchmark.java [85] query = (language.equals("PPL") ? PPL_QUERIES : SQL_QUERIES).get(queryPattern); +if (query == null) { + throw new IllegalArgumentException("Unknown queryPattern: " + queryPattern + " for language: " + language); +} Suggestion importance[1-10]: 4 __ Why: The @Param annotation constrains queryPattern to known values matching the map keys, so a null return is unlikely in practice. Still, adding a null-check improves robustness and error clarity in benchmark setup.	Low

Suggestions up to commit 5cd723f

Category	Suggestion	Impact
Possible issue	Map SQL parse/validation errors to syntax exceptions The SyntaxCheckException catch block re-throws without wrapping, but the SQLStrategy throws generic Exception types (e.g., SqlParseException, ValidationException) that will be wrapped in IllegalStateException instead of being surfaced as syntax errors. SQL parse/validation errors should also be mapped to SyntaxCheckException or a similar user-friendly exception rather than IllegalStateException. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [68-72] } catch (SyntaxCheckException e) { throw e; + } catch (org.apache.calcite.tools.ValidationException | org.apache.calcite.sql.parser.SqlParseException e) { + throw new SyntaxCheckException(e.getMessage(), e); } catch (Exception e) { throw new IllegalStateException("Failed to plan query", e); } Suggestion importance[1-10]: 6 __ Why: SQL parse and validation errors from SQLStrategy will be wrapped in IllegalStateException rather than surfaced as user-friendly syntax errors. Catching SqlParseException and ValidationException separately would improve error reporting, though this is a minor UX improvement rather than a critical bug.	Low
General	Preserve ORDER BY collation in SQL planning path relRoot.rel returns only the top-level relational node without applying any collation or result trimming. For queries with ORDER BY, the sort information is stored in relRoot.collation and may be lost. Consider using relRoot.project() which properly handles collation and field trimming, similar to how preserveCollation is applied in PPLStrategy. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [86-93] public RelNode plan(String query) throws Exception { try (Planner planner = Frameworks.getPlanner(context.getPlanContext().config)) { SqlNode parsed = planner.parse(query); SqlNode validated = planner.validate(parsed); RelRoot relRoot = planner.rel(validated); - return relRoot.rel; + return relRoot.project(); } } Suggestion importance[1-10]: 5 __ Why: Using relRoot.rel instead of relRoot.project() may lose ORDER BY collation information for SQL queries. The PPLStrategy has a preserveCollation method for this purpose, so the SQLStrategy should handle it consistently, though relRoot.project() handles this differently than preserveCollation.	Low

Suggestions up to commit 0ff9947

Category	Suggestion	Impact
Possible issue	Use projected RelNode to preserve correct output fields relRoot.rel discards the top-level RelRoot wrapper, which may contain important collation and field trimming information (e.g., relRoot.project() applies the correct field projection). Using relRoot.rel directly can result in extra fields being included in the output plan, especially for queries with ORDER BY. Use relRoot.project() instead to get the correctly projected RelNode. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [91-101] private RelNode planWithCalcite(String query) throws Exception { Planner planner = Frameworks.getPlanner(context.getPlanContext().config); try { SqlNode parsed = planner.parse(query); SqlNode validated = planner.validate(parsed); RelRoot relRoot = planner.rel(validated); - return optimize(relRoot.rel); + return optimize(relRoot.project()); } finally { planner.close(); } } Suggestion importance[1-10]: 7 __ Why: Using relRoot.project() instead of relRoot.rel is a well-known Calcite best practice that ensures correct field projection, especially for queries with ORDER BY. This is a legitimate correctness concern that could cause subtle bugs with extra fields in the output plan.	Medium
General	Isolate planning overhead from compile/transpile benchmarks The import java.sql.PreparedStatement is used only in compileQuery(), but the benchmark also imports UnifiedQueryTranspiler and UnifiedQueryCompiler which are used in transpileToSql() and compileQuery(). However, planner.plan(query) is called inside each benchmark method, meaning a new Calcite Planner is created and closed on every benchmark iteration. This adds significant overhead that is being measured as part of the benchmark rather than isolating the compile/transpile step. Consider planning once in @Setup and reusing the RelNode. benchmarks/src/jmh/java/org/opensearch/sql/api/UnifiedSqlQueryBenchmark.java [71-88] -import java.sql.PreparedStatement; -... +private RelNode cachedPlan; + +@Setup(Level.Invocation) +public void setUpInvocation() { + cachedPlan = planner.plan(query); +} + +@Benchmark +public RelNode planQuery() { + return planner.plan(query); +} + +@Benchmark +public String transpileToSql() { + return transpiler.toSql(cachedPlan); +} + +@Benchmark public void compileQuery() throws Exception { - RelNode plan = planner.plan(query); - try (PreparedStatement stmt = compiler.compile(plan)) { + try (PreparedStatement stmt = compiler.compile(cachedPlan)) { // Statement is auto-closed after benchmark iteration } } Suggestion importance[1-10]: 6 __ Why: This is a valid benchmark design concern — calling planner.plan(query) inside transpileToSql() and compileQuery() benchmarks conflates planning overhead with the actual operation being measured. Caching the plan at Level.Invocation setup would provide more accurate isolated measurements for transpile and compile operations.	Low
	Ensure HepPlanner uses consistent cost factory The HepPlanner requires a RelOptCluster to be set via hepPlanner.setRoot(rel), but it also needs the cluster's metadata providers to be properly initialized. More critically, the HepPlanner created here is disconnected from the Planner's cluster, which can cause metadata queries to fail at runtime. Pass the cluster from the RelNode to ensure consistency, or use RelOptUtil to apply rules within the existing planner context. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [104-110] private RelNode optimize(RelNode rel) { - HepPlanner hepPlanner = - new HepPlanner( - new HepProgramBuilder().addRuleInstance(CoreRules.AGGREGATE_CASE_TO_FILTER).build()); + HepProgram program = + new HepProgramBuilder().addRuleInstance(CoreRules.AGGREGATE_CASE_TO_FILTER).build(); + HepPlanner hepPlanner = new HepPlanner(program, null, false, + null, RelOptCostImpl.FACTORY); hepPlanner.setRoot(rel); - return hepPlanner.findBestExp(); + RelNode optimized = hepPlanner.findBestExp(); + return optimized; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about HepPlanner cluster consistency, but the improved_code only adds a different constructor call with RelOptCostImpl.FACTORY without actually addressing the cluster connectivity issue described. The fix is incomplete and introduces an import not shown in the diff.	Low
	Strengthen invalid SQL exception assertions The test only verifies that an IllegalStateException is thrown for invalid SQL, but does not verify the exception message or cause. A malformed SQL like "SELECT FROM" might actually throw a different exception type depending on Calcite's parser behavior. Consider also testing with a completely invalid string (e.g., "NOT SQL AT ALL") and verifying the exception message contains meaningful information. api/src/test/java/org/opensearch/sql/api/UnifiedSqlQueryPlannerTest.java [126-129] @Test public void testInvalidSqlThrowsException() { - assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM")); + IllegalStateException ex = + assertThrows(IllegalStateException.class, () -> planner.plan("SELECT FROM")); + assertNotNull("Exception should have a cause", ex.getCause()); } +@Test +public void testNonSqlStringThrowsException() { + assertThrows(IllegalStateException.class, () -> planner.plan("NOT SQL AT ALL")); +} + Suggestion importance[1-10]: 3 __ Why: While adding a cause check and an additional test case improves test coverage, this is a minor enhancement to test quality rather than a correctness fix. The existing test already validates the basic exception behavior.	Low

[github-actions]

Persistent review updated to latest commit 5cd723f

[github-actions]

Persistent review updated to latest commit 6bf098d

[github-actions]

Persistent review updated to latest commit 2cd1e28

[github-actions]

Persistent review updated to latest commit dbde012