Add unified query compiler API #4974
Conversation
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds UnifiedQueryCompiler to compile Calcite RelNode plans into JDBC PreparedStatements, makes UnifiedQueryContext AutoCloseable (close closes the JDBC connection), introduces a ResultSetAssertion test fixture, refactors test table infrastructure, and adds unit and integration tests exercising compilation and execution against OpenSearch. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Compiler as UnifiedQueryCompiler
participant Shuttle as RelHomogeneousShuttle
participant JDBCConn as JDBC Connection
participant Runner as RelRunner
participant DB as Database
Client->>Compiler: compile(plan)
Compiler->>Shuttle: plan.accept(shuttle)
Shuttle-->>Compiler: transformedPlan
Compiler->>JDBCConn: obtain RelRunner from connection
JDBCConn-->>Runner: RelRunner
Compiler->>Runner: toBindable(transformedPlan)
Runner-->>Compiler: PreparedStatement
Compiler-->>Client: PreparedStatement
Client->>PreparedStatement: executeQuery()
PreparedStatement->>DB: SQL execution
DB-->>Client: ResultSet
Client->>UnifiedQueryContext: close()
UnifiedQueryContext->>JDBCConn: connection.close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
dai-chen
requested review from
GumpacG,
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
penghuo,
ps48,
seankao-az and
vamsimanohar
as code owners
dai-chen
requested review from
LantaoJin,
RyanL1997,
noCharger,
qianheng-aws,
ykmr1224 and
yuancu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/src/main/java/org/opensearch/sql/api/compiler/UnifiedQueryCompiler.java (1)
34-36: Consider adding null validation in constructor for fail-fast behavior.Adding
Objects.requireNonNull(context)would provide clearer error messages if a null context is passed, rather than failing later duringcompile().🔎 Proposed fix
+import java.util.Objects; ... public UnifiedQueryCompiler(UnifiedQueryContext context) { - this.context = context; + this.context = Objects.requireNonNull(context, "context must not be null"); }integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java (1)
111-130: Consider using a more robust lazy-loading pattern for the table map.The anonymous
HashMapsubclass only overridesget(), leavingcontainsKey(),keySet(),entrySet(), andvalues()with inconsistent behavior. If Calcite's schema introspection calls these methods, it won't see dynamically-added tables.🔎 Proposed fix using getTableMap override pattern
Consider overriding
getTableMap()to return a fully-synchronized lazy map or useAbstractSchema's built-in caching:private AbstractSchema createOpenSearchSchema() { return new AbstractSchema() { private final OpenSearchClient osClient = new OpenSearchRestClient(new InternalRestHighLevelClient(client())); + private final Map<String, Table> tableCache = new ConcurrentHashMap<>(); @Override protected Map<String, Table> getTableMap() { - return new HashMap<>() { - @Override - public Table get(Object key) { - if (!super.containsKey(key)) { - String indexName = (String) key; - super.put(indexName, new OpenSearchIndex(osClient, context.getSettings(), indexName)); - } - return super.get(key); - } - }; + return tableCache; + } + + @Override + public Table getTable(String name) { + return tableCache.computeIfAbsent(name, + indexName -> new OpenSearchIndex(osClient, context.getSettings(), indexName)); } }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/README.md(2 hunks)api/build.gradle(2 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java(1 hunks)api/src/main/java/org/opensearch/sql/api/compiler/UnifiedQueryCompiler.java(1 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java(2 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java(2 hunks)api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java(1 hunks)api/src/testFixtures/java/org/opensearch/sql/api/ResultSetAssertion.java(1 hunks)integ-test/build.gradle(1 hunks)integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/main/java/org/opensearch/sql/api/compiler/UnifiedQueryCompiler.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.javainteg-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.javaapi/src/testFixtures/java/org/opensearch/sql/api/ResultSetAssertion.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/main/java/org/opensearch/sql/api/compiler/UnifiedQueryCompiler.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.javainteg-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.javaapi/src/testFixtures/java/org/opensearch/sql/api/ResultSetAssertion.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.javainteg-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Verify changes with `./gradlew :integ-test:integTest` before merge
Applied to files:
api/build.gradleinteg-test/build.gradle
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
api/README.mdapi/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.javainteg-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.javainteg-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.javaapi/src/testFixtures/java/org/opensearch/sql/api/ResultSetAssertion.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.javainteg-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.javaapi/src/testFixtures/java/org/opensearch/sql/api/ResultSetAssertion.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to integ-test/**/*IT.java : End-to-end scenarios need integration tests in `integ-test/` module
Applied to files:
integ-test/build.gradle
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Use `./gradlew :integ-test:integTest` for integration testing in OpenSearch SQL
Applied to files:
integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java
🧬 Code graph analysis (2)
api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java (2)
legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ResultSet.java (1)
ResultSet(12-54)api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java (1)
UnifiedQueryTestBase(34-131)
integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java (2)
api/src/main/java/org/opensearch/sql/api/compiler/UnifiedQueryCompiler.java (1)
UnifiedQueryCompiler(24-71)legacy/src/main/java/org/opensearch/sql/legacy/esdomain/OpenSearchClient.java (1)
OpenSearchClient(19-74)
🔇 Additional comments (21)
integ-test/build.gradle (1)
211-214: LGTM!The dependency additions are properly structured. The
testImplementationfor the api project enables integration tests to use theUnifiedQueryCompiler, and thehamcrest-coreexclusion from test fixtures prevents version conflicts with the existing hamcrest dependency (line 207).api/build.gradle (1)
8-8: LGTM!The test fixtures configuration follows Gradle best practices:
java-test-fixturesplugin enables sharing test utilities (e.g.,ResultSetAssertion) across modulestestFixturesApicorrectly exposes JUnit and Hamcrest as transitive dependencies for consuming modules likeinteg-testAlso applies to: 17-17, 22-24
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (1)
37-37: LGTM!The
AutoCloseableimplementation properly enables try-with-resources for resource management. The null checks are appropriately defensive, and the JavaDoc clearly documents the method's behavior.Also applies to: 45-55
api/src/main/java/org/opensearch/sql/api/compiler/UnifiedQueryCompiler.java (1)
47-70: LGTM!The
compile()method correctly implements the Calcite pattern for converting logical plans to executable statements:
- Uses
RelHomogeneousShuttleto transformLogicalTableScantoBindableTableScanfor execution- Properly delegates statement preparation to
RelRunner- Error handling wraps exceptions with clear messaging
The
@NonNullannotation and exception wrapping follow best practices for a public API.api/README.md (1)
7-18: LGTM!The documentation updates are clear and well-structured:
- Properly distinguishes between Language Specification (planner, transpiler) and Execution Runtime (compiler)
- Code examples demonstrate proper resource management with try-with-resources for both
UnifiedQueryContextandPreparedStatement- Complete workflow example effectively shows both transpilation and direct execution paths
Also applies to: 74-87, 89-123
api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java (2)
66-71: LGTM!The
tearDown()method properly implements test lifecycle cleanup with defensive null checking, ensuring resources are released even if test setup partially fails.
87-105: SimpleTable builder pattern creates insertion-order risk for schema columns.The
@Singular("col")annotation on the plainMap<String, SqlTypeName> schemafield does not guarantee insertion order preservation in the generated map. Lombok's @Singular does not use LinkedHashMap for plain Map types, so columns may be iterated in unpredictable order whengetRowType()builds the schema, potentially causing inconsistent behavior across test runs.Consider using
LinkedHashMap<String, SqlTypeName>explicitly to ensure column ordering remains consistent and predictable.Likely an incorrect or invalid review comment.
api/src/test/java/org/opensearch/sql/api/compiler/UnifiedQueryCompilerTest.java (4)
22-30: LGTM! Good test structure with proper resource setup.The test class follows conventions:
*Test.javasuffix, extends the shared base, and implements the fluent assertion mixin. The compiler is correctly initialized after callingsuper.setUp()to ensure context is ready.
32-46: Well-structured test with comprehensive verification.The test properly validates both schema and data. Using
try-with-resourcesfor thePreparedStatementensures cleanup. The expected rows (Bob and Charlie) correctly match records whereage > 30from the base test data.
48-58: Correct aggregation test with appropriate unordered assertion.The test correctly validates
stats count() by departmentresults. UsingcontainsInAnyOrder(viaexpectData) is the right choice since aggregation result order is non-deterministic. The expected counts (2 Engineering, 1 Sales, 1 Marketing) align with the base test data.
60-67: Error handling test validates exception propagation.The test correctly verifies that compilation failures are wrapped as
IllegalStateException. Consider usingassertThrowswith message verification for more detailed failure assertions in future tests, though the current approach is acceptable for validating the basic contract.integ-test/src/test/java/org/opensearch/sql/api/UnifiedQueryOpenSearchIT.java (5)
30-38: Well-structured integration test class.Correct naming with
*IT.javasuffix. Proper inheritance fromPPLIntegTestCasefor OpenSearch integration infrastructure and mixin implementation for fluent assertions.
40-62: Comprehensive test initialization with appropriate settings.Good use of builder pattern for context creation. Settings cover query limits, Calcite pushdown configuration, and cursor management. The dynamic schema approach in
createOpenSearchSchema()allows flexible table access.
64-69: Proper cleanup with null-safety.The
@Aftermethod ensuresUnifiedQueryContextis closed after each test, releasing resources like JDBC connections.
88-105: Validates context reusability across multiple queries.This test ensures the
UnifiedQueryContextcan be reused for multiple query executions without resource leaks or state corruption. The minimal assertion (rs.next()returning true) is appropriate for this lifecycle-focused test.
71-86: End-to-end PPL query execution test with specific data assertions.The test properly covers the full pipeline: planning, compilation, execution, and verification. Expected values are present in the test data: Amber (age 32), Hattie (age 36), and Dale (age 33) are loaded from the ACCOUNT index with ages > 30. Correct use of try-with-resources for resource management.
api/src/testFixtures/java/org/opensearch/sql/api/ResultSetAssertion.java (5)
24-34: Clean mixin interface with fluent factory method.Good design using a mixin interface pattern that test classes can implement to gain assertion capabilities without inheritance constraints.
36-49: Well-implemented column matcher.The
TypeSafeMatchercorrectly validates both column name and SQL type. ThedescribeToprovides adequate failure context.
51-64: Correct row matcher using array comparison.
Arrays.equalsprovides proper element-by-element comparison including null handling.
66-71: Concise value class with Lombok.
@Valueprovides immutability and all necessary boilerplate for use in assertions.
73-106: Fluent verifier with appropriate ordered/unordered assertions.Good design choices:
expectSchemausescontains(ordered, matching JDBC column positions), whileexpectDatausescontainsInAnyOrder(order-independent for result sets). The@SafeVarargsannotation correctly suppresses heap pollution warnings.Note:
expectDataconsumes the entireResultSetcursor. If called twice, the second call would match against an empty row list. This is acceptable for typical test patterns whereexpectDatais called once at the end of the chain.
Signed-off-by: Chen Dai <daichen@amazon.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java (1)
85-98: Use try-with-resources to ensure cleanup even on assertion failure.The test correctly addresses the previous double-close concern by creating a separate
testContextvariable. However, if the assertion at line 95 fails, the context won't be closed, leading to resource leaks in the test suite. SinceUnifiedQueryContextimplementsAutoCloseable, wrap it in try-with-resources.🔎 Proposed refactor using try-with-resources
@Test public void testContextClose() throws Exception { - // Create a separate context for this test to avoid affecting other tests - UnifiedQueryContext testContext = - UnifiedQueryContext.builder() - .language(QueryType.PPL) - .catalog("opensearch", testSchema) - .defaultNamespace("opensearch") - .build(); - - assertFalse(testContext.getPlanContext().connection.isClosed()); - testContext.close(); - assertTrue(testContext.getPlanContext().connection.isClosed()); + // Create a separate context for this test to avoid affecting other tests + try (UnifiedQueryContext testContext = + UnifiedQueryContext.builder() + .language(QueryType.PPL) + .catalog("opensearch", testSchema) + .defaultNamespace("opensearch") + .build()) { + assertFalse(testContext.getPlanContext().connection.isClosed()); + testContext.close(); + assertTrue(testContext.getPlanContext().connection.isClosed()); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*.java: UsePascalCasefor class names (e.g.,QueryExecutor)
UsecamelCasefor method and variable names (e.g.,executeQuery)
UseUPPER_SNAKE_CASEfor constants (e.g.,MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
PreferOptional<T>for nullable returns in Java
Avoid unnecessary object creation in loops
UseStringBuilderfor string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
**/*Test.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
**/*Test.java: All new business logic requires unit tests
Name unit tests with*Test.javasuffix in OpenSearch SQL
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*Test.java : Name unit tests with `*Test.java` suffix in OpenSearch SQL
Applied to files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
🔇 Additional comments (1)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java (1)
9-9: LGTM! Standard JUnit imports for boolean assertions.The imports for
assertFalseandassertTrueare necessary for the new test assertions intestContextClose().Also applies to: 11-11
1 participant
Description
This PR introduces a
UnifiedQueryCompileras part of the Unified Execution Runtime, enabling direct evaluation of PPL queries via a reference implementation. It completes the third pillar of the Unified Query API (alongside unified query planner and transpiler) and allows external consumers to execute PPL end-to-end using a Calcite-based in-memory evaluator, as described in #4782.Key Changes
UnifiedQueryCompiler: Introduces a new API that compiles Calcite logical plans into executable JDBC statements.UnifiedQueryContextlifecycle management: Implements AutoCloseable to properly manage resource lifecycle.Related Issues
Resolves #4894
Check List
--signoffor-s.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.