Add Settings to UnifiedQueryPlanner #4911
Conversation
lostella
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Settings field to UnifiedQueryPlanner, propagates Settings through its Builder and constructor, uses it when creating AstBuilder for parsing, adds a Mockito test dependency, and adds a mocked-Settings join test and payslips test table in test fixtures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (2)
22-42: Consider usinggetOrDefaultfor safer null handling.The
getSettingValuemethod returnsnullfor unregistered keys, which is fine. However, consider documenting this behavior or adding a default fallback if callers expect non-null values for all keys.@Override @SuppressWarnings("unchecked") public <T> T getSettingValue(Key key) { - return (T) settingsMap.get(key); + return (T) settingsMap.getOrDefault(key, null); // explicit null for missing keys }
77-88: Consider adding assertions to verify planning results.The tests validate that no exceptions are thrown (which directly addresses the NPE bug), but adding assertions on the returned
RelNodewould strengthen test quality. Per coding guidelines, tests should validate meaningful behavior.@Test public void testSimpleQueries() { - planner.plan("source=users | where id > 42"); - planner.plan("source=transactions | stats avg(id)"); + assertNotNull(planner.plan("source=users | where id > 42")); + assertNotNull(planner.plan("source=transactions | stats avg(id)")); } @Test public void testJoinQuery() { - planner.plan( + assertNotNull(planner.plan( "source=users | join on users.id = transactions.id transactions" - ); + )); }This would require adding the import:
import static org.junit.Assert.assertNotNull;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(7 hunks)api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java(1 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/CustomSettingsTest.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/CustomSettingsTest.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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/CustomSettingsTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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/CustomSettingsTest.java
🧠 Learnings (2)
📓 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: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
🧬 Code graph analysis (1)
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (2)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
UnifiedQueryPlanner(46-262)
⏰ 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). (1)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (4)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (3)
56-80: LGTM! Clean constructor delegation pattern with proper JavaDoc.The Settings integration is well-designed. The new constructor with Settings parameter has complete JavaDoc, and the original constructor delegates with
nullto maintain backward compatibility.
203-212: Builder method properly follows the fluent pattern with JavaDoc.The
settings()method follows the established builder pattern and includes proper JavaDoc documentation.
146-150: Verify AstBuilder handles null settings gracefully for non-join queries.The fix correctly passes
settingstoAstBuilder. Confirm thatAstBuilderdoesn't fail on code paths that don't require settings whensettingsis null (legacy constructor usage).api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (1)
69-75: Static planner initialization is acceptable here.The static
plannerinstance works becauseUnifiedQueryPlannerappears to be stateless after construction. This avoids repeated builder overhead across tests.
lostella
force-pushed
the
fix-unified-query-planner
branch
2 times, most recently
from
9b152f9 to
02e75f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (1)
16-76: Tests exercise Calcite + settings path well; consider minor polishThe test setup (custom
Settings,TestSchema, and a shared PPLUnifiedQueryPlannerwith cache enabled) does a good job of reproducing and guarding against the NPE in join planning. From a Calcite‑integration perspective this aligns nicely with the guidance to test SQL/plan generation paths for such changes.If you want small optional improvements:
- Declare
plannerasprivate static finalto make intent explicit.- Optionally assert on
RelNodenon‑null (e.g.,assertNotNull(planner.plan(...))) to make the tests’ success criteria clearer, beyond “no exception thrown”.- Add a brief class‑level JavaDoc for
CustomSettingsTestexplaining that it validates join planning with custom settings.These are non‑blocking; the current tests already cover the regression scenario. Based on learnings, this is a solid addition for Calcite path coverage.
Also applies to: 77-88
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(7 hunks)api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java(1 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/test/java/org/opensearch/sql/api/CustomSettingsTest.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/CustomSettingsTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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/CustomSettingsTest.java
🧠 Learnings (1)
📚 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java
🧬 Code graph analysis (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
⏰ 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). (1)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (2)
185-191: Buildersettings(...)wiring looks correct; just ensure all PPL callers use it where neededThe new
Builder.settings(Settings settings)and the propagation intobuild()correctly thread theSettingsinstance through toUnifiedQueryPlanner. This should address the join‑planning issue for callers that opt in via the builder.To avoid regressions in other PPL entry points, it’s worth confirming that any components that rely on Calcite/PPL join semantics are migrated to call
.settings(...)on the builder (or equivalent ctor). The repo‑wide search script above will help spot any missed sites.Also applies to: 203-213, 255-260
35-36: Unable to complete verification due to repository access limitations in the sandbox environment. To rewrite this review comment accurately, I need access to:
- The full implementation of
UnifiedQueryPlanner.java(lines 35-36, 56, 62-81, 89-91, 144-152)- The
AstBuilderconstructor and itsSettingsparameter handling- Call sites where
UnifiedQueryPlanneris constructed across the codebasePlease provide the file contents and relevant code snippets so verification can proceed.
| @Override | ||
| public List<String> getSettings() { | ||
| return settingsMap.keySet().stream().map(Key::getKeyValue).toList(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for Java version configuration in Maven pom.xml
find . -name "pom.xml" -type f | head -5 | xargs grep -l "java.version\|maven.compiler" 2>/dev/null | head -3Repository: opensearch-project/sql
Length of output: 188
🏁 Script executed:
# Search for maven compiler plugin configuration and Java version
find . -name "pom.xml" -type f | head -1 | xargs cat | grep -A 5 -B 5 "maven.compiler\|java.version" | head -30Repository: opensearch-project/sql
Length of output: 188
🏁 Script executed:
# Check if the file exists and examine its imports
cat -n api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java 2>/dev/null | head -50Repository: opensearch-project/sql
Length of output: 188
Replace Stream.toList() with Collectors.toList() for Java 11 compatibility
settingsMap.keySet().stream().map(Key::getKeyValue).toList() uses Stream.toList(), which requires Java 16+. The project must maintain Java 11 compatibility, so this will fail to compile on the supported baseline.
Fix this by switching to Collectors.toList():
+import java.util.stream.Collectors;
@@
@Override
public List<String> getSettings() {
- return settingsMap.keySet().stream().map(Key::getKeyValue).toList();
+ return settingsMap.keySet().stream()
+ .map(Key::getKeyValue)
+ .collect(Collectors.toList());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public List<String> getSettings() { | |
| return settingsMap.keySet().stream().map(Key::getKeyValue).toList(); | |
| } | |
| @Override | |
| public List<String> getSettings() { | |
| return settingsMap.keySet().stream() | |
| .map(Key::getKeyValue) | |
| .collect(Collectors.toList()); | |
| } |
🤖 Prompt for AI Agents
In api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java around lines
38 to 41, the code uses Stream.toList() which requires Java 16+; replace the
terminal call with Collectors.toList() to maintain Java 11 compatibility by
importing java.util.stream.Collectors if not present and changing the stream
terminal operation to .collect(Collectors.toList()) so the key values are
collected into a List in a Java 11-compatible way.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (1)
38-41: ReplaceStream.toList()withCollectors.toList()for Java 11 compatibility.
Stream.toList()requires Java 16+. The project must maintain Java 11 compatibility per the coding guidelines.+import java.util.stream.Collectors; ... @Override public List<String> getSettings() { - return settingsMap.keySet().stream().map(Key::getKeyValue).toList(); + return settingsMap.keySet().stream() + .map(Key::getKeyValue) + .collect(Collectors.toList()); }
🧹 Nitpick comments (3)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
89-91: Backward-compatible delegation with null settings.The existing constructor correctly delegates to the new one with
nullsettings, maintaining backward compatibility. However, note that callers using this constructor will still encounter NPE when planning join queries sinceAstBuilder.validateJoinTyperequires non-null settings.Consider adding a brief note in the JavaDoc that join queries require the settings-aware constructor.
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (2)
16-16: Add JavaDoc to the test class.Per coding guidelines, public classes should have proper JavaDoc. A brief description of what this test class validates would be helpful.
+/** + * Tests for {@link UnifiedQueryPlanner} with custom {@link Settings} configuration. + * Validates that providing Settings enables planning of join queries without NPE. + */ public class CustomSettingsTest {
77-88: Tests validate no-exception behavior; consider adding result assertions.The tests correctly verify that the NPE from issue #4910 is fixed by successfully planning queries. For stronger coverage, consider asserting that the returned
RelNodeis non-null or has expected properties.+import static org.junit.Assert.assertNotNull; ... @Test public void testSimpleQueries() { - planner.plan("source=users | where id > 42"); - planner.plan("source=transactions | stats avg(id)"); + assertNotNull(planner.plan("source=users | where id > 42")); + assertNotNull(planner.plan("source=transactions | stats avg(id)")); } @Test public void testJoinQuery() { - planner.plan( + assertNotNull(planner.plan( "source=users | join on users.id = transactions.id transactions" - ); + )); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(7 hunks)api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java(1 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/CustomSettingsTest.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/CustomSettingsTest.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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/CustomSettingsTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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/CustomSettingsTest.java
🧠 Learnings (4)
📚 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/CustomSettingsTest.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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/CustomSettingsTest.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/CustomSettingsTest.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 **/*.java : Maintain Java 11 compatibility when possible for OpenSearch 2.x
Applied to files:
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java
🧬 Code graph analysis (2)
api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (2)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
UnifiedQueryPlanner(46-262)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
🔇 Additional comments (5)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (4)
56-80: LGTM! Settings field and new constructor are well-structured.The new constructor properly stores the settings and delegates initialization. The JavaDoc is complete and describes all parameters. The field is correctly marked
finalfor immutability.
146-150: Core fix: Settings now passed to AstBuilder.This change correctly passes the settings instance to
AstBuilder, addressing the NPE reported in issue #4910 wherevalidateJoinTypecalledsettings.getSettingValue(...)on a null reference.
203-212: Builder settings method follows fluent API pattern.The setter properly follows the builder pattern with method chaining, and the JavaDoc is clear and complete.
255-260: Builder correctly propagates settings to UnifiedQueryPlanner.The
build()method now passes the configured settings to the new constructor, completing the settings integration through the builder API.api/src/test/java/org/opensearch/sql/api/CustomSettingsTest.java (1)
69-75: Static planner instance is acceptable here but consider documenting thread-safety.Using a static planner shared across tests works since the planner is immutable after construction. This pattern is efficient for test setup but assumes thread-safe read operations.
| AstStatementBuilder astStmtBuilder = | ||
| new AstStatementBuilder( | ||
| new AstBuilder(query), AstStatementBuilder.StatementBuilderContext.builder().build()); | ||
| new AstBuilder(query, settings), |
There was a problem hiding this comment.
This is the key part to resolve the issue. Can you remove the single parameter construction?
public AstBuilder(String query) {
this(query, null);
}
It's nothing but a hassle.
There was a problem hiding this comment.
I tried, but realized some Calcite tests rely on this:
- https://github.com/opensearch-project/sql/blob/main/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLChartTest.java#L303
- https://github.com/opensearch-project/sql/blob/main/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTimechartTest.java#L352
Maybe this can be done as a follow-up together with changes to the tests?
| import org.opensearch.sql.common.setting.Settings; | ||
| import org.opensearch.sql.executor.QueryType; | ||
|
|
||
| public class CustomSettingsTest { |
There was a problem hiding this comment.
No need to add a new test file. Better to reuse the UnifiedQueryPlannerTest with mock: @Mock private Settings settings;
| this.queryType = queryType; | ||
| this.parser = buildQueryParser(queryType); | ||
| this.config = buildCalciteConfig(rootSchema, defaultPath); | ||
| this(queryType, rootSchema, defaultPath, null); |
There was a problem hiding this comment.
I prefer to just delete this 3-arguments construction method. But it's fine to keep for compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java (1)
203-218: Add assertion to verify the plan was created successfully.The test method verifies that planning a join query doesn't throw an exception, but unlike other test methods in this class, it doesn't assert on the result. Consider adding an assertion for consistency and to explicitly verify the plan was created.
@Test public void testJoinQuery() { when(testSettings.getSettingValue(Settings.Key.CALCITE_SUPPORT_ALL_JOIN_TYPES)) .thenReturn(true); UnifiedQueryPlanner planner = UnifiedQueryPlanner.builder() .language(QueryType.PPL) .catalog("opensearch", testSchema) .defaultNamespace("opensearch") .settings(testSettings) .build(); - planner.plan( + RelNode plan = planner.plan( "source = index | join on index.id = index2.id index2" ); + assertNotNull("Plan should be created for join query", plan); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/build.gradle(1 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(6 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java(5 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.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/UnifiedQueryPlannerTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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/UnifiedQueryPlannerTest.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: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.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 **/*.java : Document Calcite-specific workarounds in code
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
🧬 Code graph analysis (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
⏰ 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). (1)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (9)
api/build.gradle (1)
18-18: LGTM!The addition of
mockito-junit-jupiteris appropriate to support the@RunWith(MockitoJUnitRunner.class)annotation used in the new test. Version is correctly aligned withmockito-core.api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java (3)
23-30: LGTM!Proper setup for Mockito integration with JUnit 4 using
@RunWith(MockitoJUnitRunner.class).
49-61: LGTM!The
index2table with matchingidcolumn enables proper join query testing.
74-75: LGTM!Mock field properly declared for injection by
MockitoJUnitRunner.api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (5)
56-57: LGTM!Field declaration follows Java naming conventions and is appropriately marked as
private final.
62-80: LGTM!Constructor properly documented with JavaDoc. The design allows
settingsto be nullable, which maintains backward compatibility for non-join queries while enabling join query planning when Settings is provided.
133-147: Core fix for the NPE issue.Passing
settingstoAstBuilderresolves theNullPointerExceptioninAstBuilder.validateJoinTypewhen processing join queries. This is the key change that addresses issue #4910.
192-201: LGTM!Builder method properly implements the fluent API pattern with complete JavaDoc documentation as per coding guidelines.
244-249: LGTM!The
build()method correctly passes thesettingsfield to the new constructor, completing the Settings propagation chain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java (2)
204-209: Fix indentation to match existing tests.The builder chain indentation (6 spaces) is inconsistent with the rest of the file, where builder chains use 12 spaces (e.g., lines 78-81).
Apply this diff to fix the indentation:
- UnifiedQueryPlanner planner = UnifiedQueryPlanner.builder() - .language(QueryType.PPL) - .catalog("opensearch", testSchema) - .defaultNamespace("opensearch") - .settings(testSettings) - .build(); + UnifiedQueryPlanner planner = + UnifiedQueryPlanner.builder() + .language(QueryType.PPL) + .catalog("opensearch", testSchema) + .defaultNamespace("opensearch") + .settings(testSettings) + .build();
211-211: Add an explicit assertion for the test result.The test should explicitly assert that the plan is created successfully, rather than relying on the implicit "no exception thrown" behavior.
Apply this diff to add an assertion:
- planner.plan("source = index | join on index.id = index2.id index2"); + RelNode plan = planner.plan("source = index | join on index.id = index2.id index2"); + assertNotNull("Plan should be created for join query", plan);
📜 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/UnifiedQueryPlannerTest.java(4 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/UnifiedQueryPlannerTest.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/UnifiedQueryPlannerTest.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/UnifiedQueryPlannerTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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/UnifiedQueryPlannerTest.java
🧠 Learnings (2)
📚 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/UnifiedQueryPlannerTest.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/UnifiedQueryPlannerTest.java
🔇 Additional comments (3)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java (3)
22-24: LGTM! Mockito setup is correct.The Mockito imports and @RunWith annotation are properly configured to enable mock object usage in the test class.
Also applies to: 26-26, 29-29
48-60: LGTM! Second table supports join testing.The addition of "index2" with id and value columns provides the necessary structure for testing join queries.
73-73: Verify that the testSettings mock is passed to UnifiedQueryPlanner and confirm mock configuration needs.The mock Settings field is not configured with any behavior. However, the primary concern is whether
testSettingsis actually being passed to theUnifiedQueryPlannerinstance during the test. Issue #4910 reports thatSettingsis null whenAstBuilder.validateJoinTypeexecutes, suggesting the mock may not be injected into the planner. Verify that:
- The planner is initialized with
testSettingsas a constructor argument or via method call- If the mock is used, confirm whether it needs behavior configuration (e.g.,
when(testSettings.getSettingValue(...)).thenReturn(...)) for the join validation to work correctly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
56-57: Consider adding JavaDoc for consistency.While not a public API element, other fields in this class include JavaDoc comments. Adding a brief JavaDoc comment for the
settingsfield would maintain documentation consistency.Example:
+ /** Configuration settings for query processing and feature flags. */ private final Settings settings;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(6 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
🧠 Learnings (3)
📚 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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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 **/*.java : Document Calcite-specific workarounds in code
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
🧬 Code graph analysis (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
🔇 Additional comments (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (2)
132-134: Key fix for join query NPE - LGTM with dependency on settings validation.This change correctly threads
SettingstoAstBuilder, which resolves the NPE reported in issue #4910 wherevalidateJoinTypeneeds to callsettings.getSettingValue(). The implementation is correct, assumingsettingsis properly initialized (see earlier comment about null validation).
187-197: Builder settings method looks good; consider validation in build().The
settings()method is well-documented and follows the fluent builder pattern correctly. However,build()at line 243 passes the potentially nullsettingsto the constructor without validation.If
Settingsis optional for backward compatibility or simple queries, this is acceptable. However, ifSettingsis required for join queries to work (per issue #4910), consider whetherbuild()should validate its presence similar to howqueryTypeis validated at line 240:Objects.requireNonNull(settings, "Must specify settings before build");This relates to the earlier comment about null handling in the constructor.
Should
Settingsbe treated as:
- Required: validate in
build()and constructor- Optional: document the limitation and ensure graceful degradation or clear error messages when settings-dependent features (like joins) are used without settings
Also applies to: 243-243
| public UnifiedQueryPlanner( | ||
| QueryType queryType, SchemaPlus rootSchema, String defaultPath, Settings settings) { | ||
| this.queryType = queryType; | ||
| this.parser = buildQueryParser(queryType); | ||
| this.config = buildCalciteConfig(rootSchema, defaultPath); | ||
| this.settings = settings; | ||
| } |
There was a problem hiding this comment.
Validate Settings parameter or document null handling.
The constructor accepts Settings but does not validate whether it's null. According to issue #4910, the NPE occurs when validateJoinType calls settings.getSettingValue(...) on a null settings object. While this PR threads Settings through the planner, it does not prevent null from being passed, which would cause the same NPE for join queries.
Consider one of the following approaches:
- Require non-null Settings (recommended if Settings is always needed):
public UnifiedQueryPlanner(
QueryType queryType, SchemaPlus rootSchema, String defaultPath, Settings settings) {
+ this.settings = Objects.requireNonNull(settings, "Settings cannot be null");
this.queryType = queryType;
this.parser = buildQueryParser(queryType);
this.config = buildCalciteConfig(rootSchema, defaultPath);
- this.settings = settings;
}- Allow null but document the limitation (if Settings is optional for simple queries):
- Update JavaDoc to state: "settings may be null; join queries will fail if settings is not provided"
- Consider defensive checks in
parse()method
Based on coding guidelines: validate inputs and use proper error handling.
🤖 Prompt for AI Agents
In api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java around
lines 70 to 76, the constructor accepts a Settings parameter but does not
validate it, leading to NPEs later when methods like validateJoinType call
settings.getSettingValue(...). Fix by requiring a non-null Settings: validate
the settings argument in the constructor (throw
IllegalArgumentException/NullPointerException with a clear message if null) and
update the constructor JavaDoc to state Settings is required; alternatively, if
Settings must remain optional, add a JavaDoc note that settings may be null and
add defensive null checks where settings is used (e.g., in
parse()/validateJoinType) to avoid NPEs.
Signed-off-by: Lorenzo Stella <[email protected]>
Signed-off-by: Lorenzo Stella <[email protected]>
Signed-off-by: Lorenzo Stella <[email protected]>
Signed-off-by: Lorenzo Stella <[email protected]>
Signed-off-by: Lorenzo Stella <[email protected]>
lostella
force-pushed
the
fix-unified-query-planner
branch
from
431cc3f to
2f46c5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
56-76: Enforce or document non-nullSettingsto fully avoid join NPEs.The original bug was a NPE when
AstBuilder.validateJoinTypecalledsettings.getSettingValue(...)on a nullSettings. With the new field and constructor, that NPE is prevented only if callers always pass a non-nullSettings. However:
Builder.settings(Settings)accepts null and just assigns it.Builder.build()passessettingsthrough without validation.- The
UnifiedQueryPlannerconstructor storessettingswithout null checks.Any caller using
builder()or the 4‑arg constructor without providingSettingswill still hit the same NPE when planning join queries.To close the hole, consider:
- Preferred: Require non-null
Settingsat construction time and fail fast:- public UnifiedQueryPlanner( - QueryType queryType, SchemaPlus rootSchema, String defaultPath, Settings settings) { - this.queryType = queryType; - this.parser = buildQueryParser(queryType); - this.config = buildCalciteConfig(rootSchema, defaultPath); - this.settings = settings; - } + public UnifiedQueryPlanner( + QueryType queryType, SchemaPlus rootSchema, String defaultPath, Settings settings) { + this.settings = Objects.requireNonNull(settings, "Settings must not be null"); + this.queryType = queryType; + this.parser = buildQueryParser(queryType); + this.config = buildCalciteConfig(rootSchema, defaultPath); + }Optionally also validate in
Builder.build():- public UnifiedQueryPlanner build() { - Objects.requireNonNull(queryType, "Must specify language before build"); + public UnifiedQueryPlanner build() { + Objects.requireNonNull(queryType, "Must specify language before build"); + Objects.requireNonNull(settings, "Must specify settings before build"); SchemaPlus rootSchema = CalciteSchema.createRootSchema(true, cacheMetadata).plus(); catalogs.forEach(rootSchema::add); return new UnifiedQueryPlanner(queryType, rootSchema, defaultNamespace, settings); }
- Alternative: If you intentionally want
Settingsto be optional for non-join queries, then document this contract clearly in the constructor andBuilder.settingsJavaDoc, and add defensive handling in the join validation path so a missingSettingsresults in a clear, non-NPE failure.As per coding guidelines, constructor inputs should be validated or their nullability/limitations explicitly documented.
Also applies to: 174-176, 193-196, 239-244
🧹 Nitpick comments (1)
api/build.gradle (1)
16-20: Confirm need formockito-junit-jupiteralongside JUnit 4.This module already uses JUnit 4 (
junit:4.13.2) andmockito-core.mockito-junit-jupiteris only needed for JUnit 5 Jupiter-based tests (@ExtendWith(MockitoExtension.class)). If you don't actually have Jupiter tests in this module, you can drop this dependency to keep the test classpath lean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/build.gradle(1 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(6 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java(3 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/UnifiedQueryTestBase.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
⚙️ CodeRabbit configuration file
**/*.java: - 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 methods are under 20 lines with single responsibility
- Verify 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/UnifiedQueryTestBase.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify test coverage for new business logic
- Check test naming follows conventions (*Test.java for unit, *IT.java for integration)
- 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/UnifiedQueryTestBase.java
🧠 Learnings (4)
📚 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/UnifiedQueryTestBase.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/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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 **/*.java : Document Calcite-specific workarounds in code
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.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: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
🧬 Code graph analysis (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
🔇 Additional comments (2)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java (1)
34-35: Newpayslipstest table wiring looks good.Adding
payslipsintogetTableMap()and definingcreatePayslipsTable()mirrors the existingemployeessetup, usesMap.of/List.ofconsistently, and provides a clear join target for planner tests. No issues from a schema or style standpoint.Also applies to: 61-73
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (1)
129-135: WiringSettingsintoAstBuildercorrectly targets the join-planning bug.Using
new AstBuilder(query, settings)inparse()ensures the planner’sSettingsinstance is available to AST validation (including join type checks), which is the right direction for fixing the original NPE path. Oncesettingsis guaranteed non-null (or its absence is clearly handled), this should make join planning robust.
There was a problem hiding this comment.
Thanks for proposing this fix! I'm currently working on a new generic Context abstraction to handle configs required by unified query components (e.g., planner, transpiler, compiler). This will decouple from the OpenSearch API like Settings. I'll share an update once I have a working implementation—if successful, I may submit a PR. Thanks!
|
Draft PR #4933. Feel free to comment. Thanks! |
|
Hi @lostella My PR has been merged. Just a heads up it includes breaking change to the experimental API. Please update your code after pulling the latest artifact. Thanks! |
3 participants
Description
settingssetter method toBuilderclass forUnifiedQueryPlannerSettingsobject in constructingUnifiedQueryPlannerandAstBuidlerjoinPPL queriesRelated Issues
Resolves #4910
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.