Extract unified query context for shared config management #4933
Conversation
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a reusable, session-scoped Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UnifiedQueryContext
participant UnifiedQueryPlanner
participant AstBuilder
participant CalcitePlanContext
Note over Client,UnifiedQueryContext: One-time / reusable initialization
Client->>UnifiedQueryContext: Builder(language, catalogs, defaultNamespace, settings).build()
UnifiedQueryContext-->>Client: UnifiedQueryContext
Note over Client,UnifiedQueryPlanner: Per-plan usage
Client->>UnifiedQueryPlanner: new UnifiedQueryPlanner(context)
Client->>UnifiedQueryPlanner: plan(query)
UnifiedQueryPlanner->>UnifiedQueryContext: getSettings()
UnifiedQueryPlanner->>UnifiedQueryContext: getPlanContext()
UnifiedQueryPlanner->>AstBuilder: new AstBuilder(query, settings)
AstBuilder->>CalcitePlanContext: analyze(parse tree) using planContext/schema
CalcitePlanContext-->>AstBuilder: metadata/rel info
AstBuilder-->>UnifiedQueryPlanner: RelNode
UnifiedQueryPlanner-->>Client: RelNode (planned)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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 |
Signed-off-by: Chen Dai <[email protected]>
dai-chen
changed the title
Signed-off-by: Chen Dai <[email protected]>
Signed-off-by: Chen Dai <[email protected]>
dai-chen
changed the title
dai-chen
added
bug
dai-chen
requested review from
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
anirudha,
derek-ho,
joshuali925,
kavithacm,
mengweieric,
penghuo,
ps48,
seankao-az and
vamsimanohar
as code owners
dai-chen
requested review from
GumpacG,
LantaoJin,
RyanL1997,
acarbonetto,
forestmvey,
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/README.md (1)
77-78: Minor: Comment says "Parse" butplan()does both parsing and planning.The comment at Step 3 says "Parse PPL query into logical plan" but the
plan()method internally does parsing, analysis, and plan generation. Consider updating to "Plan PPL query into logical plan" for consistency with the method name.-// Step 3: Parse PPL query into logical plan +// Step 3: Plan PPL query into logical plan RelNode plan = planner.plan("source = employees | where age > 30");api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java (1)
64-72: Consider clarifying test name or splitting the test.The test creates a
UnifiedQueryContextwith SQL (which succeeds), but theIllegalArgumentExceptionis thrown when constructingUnifiedQueryPlanner. The test nametestUnsupportedQueryTypecould betestPlannerRejectsUnsupportedQueryTypeto clarify where the validation occurs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/README.md(2 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java(1 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java(3 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java(1 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java(3 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.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/UnifiedQueryPlannerTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.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.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.javaapi/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.javaapi/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/UnifiedQueryPlannerTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.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.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java
🧠 Learnings (5)
📚 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.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: Follow existing patterns in `CalciteRelNodeVisitor` and `CalciteRexNodeVisitor` for Calcite integration
Applied to files:
api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java
📚 Learning: 2025-12-11T05:27:39.831Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.831Z
Learning: In opensearch-project/sql, for SEMI and ANTI join types in CalciteRelNodeVisitor.java, the `max` option has no effect because these join types only use the left side to filter records based on the existence of matches in the right side. The join results are identical regardless of max value (max=1, max=2, or max=∞). The early return for SEMI/ANTI joins before processing the `max` option is intentional and correct behavior.
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 **/*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
🧬 Code graph analysis (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (2)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
CalcitePlanContext(30-137)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java (1)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)
🔇 Additional comments (13)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (3)
144-157: Consider returning an unmodifiable list fromgetSettings().The
getSettings()method returnsList.copyOf(settings.entrySet()), which is already unmodifiable. This is a good practice for immutability.
31-48: Well-designed session-level context abstraction.The
UnifiedQueryContextclass is well-structured with proper JavaDoc, immutability via@Value, and a clean Builder API. This design effectively centralizes configuration and resolves the Settings null issue from #4910.
173-186: Usesplit("\\.")for literal dot splitting.
String.split()accepts a regex pattern. Using"."matches any character, which will produce unexpected results. For a literal dot, use"\\.".private SchemaPlus findSchemaByPath(SchemaPlus rootSchema, String defaultPath) { if (defaultPath == null) { return rootSchema; } SchemaPlus current = rootSchema; - for (String part : defaultPath.split("\\.")) { + for (String part : defaultPath.split("\\.")) { current = current.getSubSchema(part); if (current == null) { throw new IllegalArgumentException("Invalid default catalog path: " + defaultPath); } } return current; }Wait - I see the code already has
"\\.". Let me re-check the provided code at line 179. The code showsdefaultPath.split("\\.")which is correct. Disregard this comment.api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java (1)
38-44: Clean migration to context-based planner initialization.The test base correctly builds
UnifiedQueryContextwith the required configuration and initializes the planner using the new constructor. This aligns with the API changes and provides a consistent test setup.api/README.md (1)
20-35: Documentation accurately reflects the new API design.The
UnifiedQueryContextsection clearly explains the session-level abstraction and provides a complete example of context configuration. The workflow is well-documented with logical step progression.api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java (2)
43-56: Good addition: Test validates the join query fix from issue #4910.This test directly validates the fix for the linked bug where
UnifiedQueryPlannerfailed with NPE for join queries due to null Settings. The test ensures join queries can be planned successfully.
30-41: Tests follow consistent patterns and are well-structured.The tests demonstrate independent, focused test cases with proper setup and assertions. The migration to context-based construction is consistent across all test methods.
api/src/test/java/org/opensearch/sql/api/UnifiedQueryContextTest.java (2)
18-34: Good validation of default settings and context construction.The test verifies that default system limits from
SysLimit.DEFAULTare properly applied when no custom settings are specified. This ensures backward compatibility with existing behavior.
50-57: Good edge case testing for invalid settings.Testing that invalid setting names throw
IllegalArgumentExceptionensures the builder provides clear feedback for configuration errors.api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java (4)
46-49: Clean constructor that centralizes context dependency.The constructor properly extracts configuration from
UnifiedQueryContext, ensuring that Settings is never null. This directly addresses the root cause of issue #4910 where NPE occurred during join query planning.
76-89: Settings properly passed to AstBuilder - resolves #4910.The
parse()method now correctly passescontext.getSettings()toAstBuilder, ensuring thatvalidateJoinType()has access to Settings and won't throw NPE for join queries.
91-93: Analysis correctly uses plan context from unified context.The
analyze()method properly delegates to theCalcitePlanContextobtained fromUnifiedQueryContext, maintaining consistency with the new architecture.
25-39: Class structure is cleaner after refactoring.The
UnifiedQueryPlanneris now focused solely on query planning, with configuration management delegated toUnifiedQueryContext. The class follows the existing patterns for Calcite integration.
Signed-off-by: Chen Dai <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (4)
57-67: Consider seeding additional settings (e.g.,PPL_SYNTAX_LEGACY_PREFERRED) to avoid nullsRight now the builder’s settings map only seeds the three SysLimit‑related keys. When this
Settingsinstance is passed intoCalcitePlanContext.run(...),getSettingValue(PPL_SYNTAX_LEGACY_PREFERRED)will returnnullunless the caller explicitly overrides it viasetting(...). BecauseisLegacyPreferred()unboxes the thread‑localBoolean, a missing value can result in an NPE at runtime.To match the documented default (
true) and avoid surprises, consider seeding this key as well (and any other settings that are read without null‑handling):-import static org.opensearch.sql.common.setting.Settings.Key.PPL_JOIN_SUBSEARCH_MAXOUT; -import static org.opensearch.sql.common.setting.Settings.Key.PPL_SUBSEARCH_MAXOUT; -import static org.opensearch.sql.common.setting.Settings.Key.QUERY_SIZE_LIMIT; +import static org.opensearch.sql.common.setting.Settings.Key.PPL_JOIN_SUBSEARCH_MAXOUT; +import static org.opensearch.sql.common.setting.Settings.Key.PPL_SUBSEARCH_MAXOUT; +import static org.opensearch.sql.common.setting.Settings.Key.PPL_SYNTAX_LEGACY_PREFERRED; +import static org.opensearch.sql.common.setting.Settings.Key.QUERY_SIZE_LIMIT; @@ - private final Map<Settings.Key, Object> settings = - new HashMap<Settings.Key, Object>( - Map.of( - QUERY_SIZE_LIMIT, SysLimit.DEFAULT.querySizeLimit(), - PPL_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.subsearchLimit(), - PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.joinSubsearchLimit())); + private final Map<Settings.Key, Object> settings = + new HashMap<>( + Map.of( + QUERY_SIZE_LIMIT, SysLimit.DEFAULT.querySizeLimit(), + PPL_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.subsearchLimit(), + PPL_JOIN_SUBSEARCH_MAXOUT, SysLimit.DEFAULT.joinSubsearchLimit(), + PPL_SYNTAX_LEGACY_PREFERRED, Boolean.TRUE));This keeps the builder self‑contained and safer for PPL usage even when the caller doesn’t know to override syntax preferences.
115-128:setting(String, Object)is flexible but type‑unsafe; consider tightening or documenting expected typesAllowing
Object valuewith an unchecked cast ingetSettingValuemakes it easy for callers to accidentally pass the wrong type for a given key (e.g., aStringforQUERY_SIZE_LIMIT), leading to ClassCastExceptions deep in planning.Two lightweight options:
- Add explicit type expectations to the Javadoc for common keys (limits, booleans) so API users know what to pass.
- Or, if you want stronger guarantees, special‑case a few critical keys and validate the runtime type before putting them into the map, failing fast with a clearer message.
Both would improve debuggability without complicating the builder API.
145-157: Clarify the contract ofSettings.getSettings()for this anonymous implementationHere
getSettings()returns aListofMap.Entry<Key,Object>. That’s fine for the current usages (which mostly rely ongetSettingValue), but other callers might reasonably expect a list of concrete “setting descriptors” rather than raw map entries.To prevent misuse:
- Either document in a class‑level or method‑level comment that, for
UnifiedQueryContext’sSettings,getSettings()is only intended for inspection and returns(Key, value)pairs, or- If there are no current consumers in the unified API path, you could consider throwing
UnsupportedOperationExceptionfromgetSettings()to make the contract explicit.This keeps behavior predictable for anyone reusing
UnifiedQueryContext.settingsoutside the immediate planner flow.
174-187: Handle empty default namespace more gracefully
findSchemaByPathtreatsnullas “use root”, which is good, but an empty string (e.g.,defaultNamespace("")) ends up splitting into[""], callinggetSubSchema(""), and throwingIllegalArgumentException.If an empty namespace is a realistic input (e.g., wired from user config or tests), it might be more ergonomic to treat blank the same as
null:- private SchemaPlus findSchemaByPath(SchemaPlus rootSchema, String defaultPath) { - if (defaultPath == null) { + private SchemaPlus findSchemaByPath(SchemaPlus rootSchema, String defaultPath) { + if (defaultPath == null || defaultPath.isBlank()) { return rootSchema; }Otherwise, consider documenting in
defaultNamespace(...)’s Javadoc thatnullmeans “root schema” and non‑empty, dot‑separated paths are required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/README.md(2 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/README.md
🧰 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/UnifiedQueryContext.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/UnifiedQueryContext.java
🧬 Code graph analysis (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (2)
common/src/main/java/org/opensearch/sql/common/setting/Settings.java (1)
Settings(17-105)core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java (1)
CalcitePlanContext(30-137)
⏰ 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: build-linux (25, integration)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: security-it-linux (21)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (3)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (3)
31-44: UnifiedQueryContext abstraction looks solid and aligns with the PR’s goalsBundling
CalcitePlanContextandSettingsinto an immutable, session‑scopedUnifiedQueryContextis a clean way to centralize config and should address the previousSettings == nullNPEs in unified planning. No issues with the overall shape of this API from this file alone.
135-143: Builderbuild()flow is clean and correctly reuses a singleSettingsinstance
Objects.requireNonNull(queryType, ...)ensures the language is always set, and reusing theSettingsreturned frombuildSettings()for bothSysLimit.fromSettings(...)and theUnifiedQueryContextavoids the double‑allocation/identity mismatch noted in earlier reviews. No further issues here.
160-172: FrameworkConfig construction is straightforward and respects optional default namespaceThe
buildFrameworkConfig()implementation wiresparserConfig,defaultSchema, trait defs, and program in a conventional Calcite way, and the use ofcacheMetadatawhen creating the root schema is a nice touch. No correctness or style issues spotted here.
Signed-off-by: Chen Dai <[email protected]>
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)
36-43: New join-planning test correctly exercises the NPE path; consider asserting plan shapeThis test now hits the PPL
joincode path using the documented syntax (join left = l right = r ON l.id = r.age catalog.employees), which is exactly what previously triggered theSettings-related NPE. That’s a solid regression test.If you want to harden it further, you could add a lightweight assertion on the
RelNodestructure (for example, verify that there is aLogicalJoinin the tree or that the plan’s root matches the expected operator) to better cover Calcite plan generation, not just “no exception”. Based on learnings, this would give stronger coverage of SQL generation and optimization paths for Calcite integration.
45-57: UnifiedQueryContext usage for default namespace is consistentConstructing a
UnifiedQueryContextwith.catalog("opensearch", testSchema)and.defaultNamespace("opensearch"), then instantiating a localUnifiedQueryPlanner, cleanly validates both fully qualified (source = opensearch.employees) and default-namespace (source = employees) resolution. The pattern is clear and matches the new unified context design.If this pattern repeats in more tests, consider extracting a small helper in
UnifiedQueryTestBaseto build a PPL planner with a given default namespace to reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java(3 hunks)api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java(2 hunks)api/src/test/java/org/opensearch/sql/api/transpiler/UnifiedQueryTranspilerTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/test/java/org/opensearch/sql/api/UnifiedQueryTestBase.java
🧰 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/transpiler/UnifiedQueryTranspilerTest.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/test/java/org/opensearch/sql/api/transpiler/UnifiedQueryTranspilerTest.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/transpiler/UnifiedQueryTranspilerTest.javaapi/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/transpiler/UnifiedQueryTranspilerTest.javaapi/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.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/test/java/org/opensearch/sql/api/transpiler/UnifiedQueryTranspilerTest.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 **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL
Applied to files:
api/src/test/java/org/opensearch/sql/api/transpiler/UnifiedQueryTranspilerTest.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/transpiler/UnifiedQueryTranspilerTest.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: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (8)
api/src/test/java/org/opensearch/sql/api/transpiler/UnifiedQueryTranspilerTest.java (2)
29-29: LGTM: Test input correctly uses catalog-qualified source.The update from "employees" to "catalog.employees" aligns the test input with the expected SQL output that already includes catalog qualification, ensuring the test validates catalog-qualified source handling.
40-40: LGTM: Catalog qualification applied consistently.The update maintains consistency with the first test case while preserving the custom dialect validation logic. The catalog-qualified source correctly aligns with the expected SQL output.
api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerTest.java (6)
30-34: Catalog-qualified base PPL planning test looks goodUsing
source = catalog.employees | eval f = abs(id)keeps this test aligned with the new catalog-based setup and still exercises the basic PPL planning path throughUnifiedQueryPlanner. No issues from a correctness or readability standpoint.
59-75: Multi-level default namespace coverage looks correctThe
testDeepSchemaplus:
.catalog("catalog", testDeepSchema).defaultNamespace("catalog.opensearch")nicely exercise multi-level namespace resolution. The positive assertions for
catalog.opensearch.employeesand bareemployees, along with theIllegalStateExceptionexpectation forsource = opensearch.employees, clearly document the intended Calcite behavior when the default root schema isn’tcatalog. This is a good targeted regression test for namespace handling.
77-91: Multiple-catalogs test validates cross-catalog lookup as intendedThe context with two catalogs:
catalog1andcatalog2both bound totestSchemacombined with the query
source = catalog1.employees | lookup catalog2.employees id | eval f = abs(id)provides a clear validation of cross-catalog lookup behavior under the unified context model. The assertions are minimal but sufficient for verifying planner support for multiple catalog registrations.
93-107: Default-namespace behavior across multiple catalogs is well coveredHere,
.defaultNamespace("catalog2")plus the query:source = catalog1.employees | lookup employees id | eval f = abs(id)correctly tests that unqualified
employeesresolves againstcatalog2. This is a nice complement to the previous test and ensures default-namespace resolution works even in a multi-catalog environment.
109-112: Unsupported statement-type test remains valid after catalog renamingUpdating the query to
explain source = catalog.employeeskeeps the intent of this test (rejecting unsupported statement types) while aligning with the new catalog naming convention. ExpectingIllegalStateExceptionis still appropriate here.
114-117: Syntax error propagation test correctly targets parser failureChanging the query to
source = catalog.employees | evalpreserves the original goal: trigger a syntax error during parsing and verify thatSyntaxCheckExceptionis propagated by the planner. This continues to exercise the error path with the new catalog-qualified source.
api/README.md
Outdated
| ### UnifiedQueryPlanner | ||
| ### UnifiedQueryContext | ||
|
|
||
| `UnifiedQueryContext` is a reusable session-level abstraction shared across unified query components (planner, compiler, etc.). It bundles `CalcitePlanContext` and `Settings` into a single object, centralizing configuration for all unified query operations. |
There was a problem hiding this comment.
High-level Q: What session do you mean? I mean, how to define a session start and close? I didn't see any session management code at a glance.
There was a problem hiding this comment.
I was thinking about session in SQL/Spark. Since we've not clearly defined it, let me remove it to avoid confusion. Thanks!
Signed-off-by: Chen Dai <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (1)
61-67: Snapshot the settings map so multiplebuild()calls don’t share mutable stateRight now the anonymous
Settingsreturned bybuildSettings()closes over theBuilder.settingsmap directly. If a caller reuses the builder and mutates settings betweenbuild()calls, earlierUnifiedQueryContextinstances will “see” those later changes viagetSettingValue/getSettings, which is surprising for typical builder semantics.It’s safer to snapshot the map at build time and have each
Settingsinstance own its own immutable view:- private Settings buildSettings() { - return new Settings() { + private Settings buildSettings() { + // Snapshot builder settings at build time so each context is isolated + Map<Settings.Key, Object> snapshot = Map.copyOf(settings); + return new Settings() { @Override @SuppressWarnings("unchecked") public <T> T getSettingValue(Key key) { - return (T) settings.get(key); + return (T) snapshot.get(key); } @Override public List<?> getSettings() { - return List.copyOf(settings.entrySet()); + return List.copyOf(snapshot.entrySet()); } }; }This keeps the current behavior for a single build while avoiding aliasing if the builder is reused later.
Also applies to: 122-128, 145-157
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/README.md(2 hunks)api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java(1 hunks)
🧰 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/UnifiedQueryContext.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/UnifiedQueryContext.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 (21, doc)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- 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 (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (2)
api/src/main/java/org/opensearch/sql/api/UnifiedQueryContext.java (1)
31-44: UnifiedQueryContext API and builder wiring look solid and match the PR goalsThe class cleanly encapsulates
CalcitePlanContextandSettings, and the builder flow (language/catalog/defaultNamespace/cacheMetadata/setting → build) is consistent and well‑documented. UsingSysLimit.DEFAULTto seed planning‑relevant settings and then feeding a singleSettingsinstance into bothSysLimit.fromSettings(...)and theUnifiedQueryContextaddresses the original NPE risk around missing settings while keeping configuration explicit. From a Java style standpoint (naming, JavaDoc on public API, method sizes), this file looks good to me.Also applies to: 51-67, 69-143
api/README.md (1)
20-25: Docs correctly reflect the new context‑centric APIThe updated README clearly introduces
UnifiedQueryContext, shows building it with catalogs,QueryType, default namespace, and settings, and then using it to construct aUnifiedQueryPlannerthat’s reused across multiple plans. The complete workflow example is consistent with the new constructor signature and resolves the earlier ambiguity around “session” by framing the context as a reusable configuration object. From an integration‑guide perspective, this aligns well with the new public API.Also applies to: 27-35, 37-48, 64-88
* Extract UnifiedQueryPlanner.Builder to UnifiedQueryContext Signed-off-by: Chen Dai <[email protected]> * Remove backward compatibility code Signed-off-by: Chen Dai <[email protected]> * Refactor unified query context with setting setter Signed-off-by: Chen Dai <[email protected]> * Initialize unified query context with default system limits Signed-off-by: Chen Dai <[email protected]> * Refactor setting map read Signed-off-by: Chen Dai <[email protected]> * Update javadoc and rename queryType to language Signed-off-by: Chen Dai <[email protected]> * Address AI comments Signed-off-by: Chen Dai <[email protected]> * Reuse context in test base class Signed-off-by: Chen Dai <[email protected]> * Remove session in doc Signed-off-by: Chen Dai <[email protected]> --------- Signed-off-by: Chen Dai <[email protected]> (cherry picked from commit 297074c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…4970) * Extract UnifiedQueryPlanner.Builder to UnifiedQueryContext * Remove backward compatibility code * Refactor unified query context with setting setter * Initialize unified query context with default system limits * Refactor setting map read * Update javadoc and rename queryType to language * Address AI comments * Reuse context in test base class * Remove session in doc --------- (cherry picked from commit 297074c) Signed-off-by: Chen Dai <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
4 participants
Description
This PR introduces
UnifiedQueryContext, a reusable abstraction shared across unified query components (parser, planner, compiler, etc.). It centralizes configuration by constructing and bundlingCalcitePlanContextandSettingsinto a single object. As a result, all unified query components can now read required configuration explicitly, resolving the configuration propagation issue tracked #4910.Related 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.