Separate explain mode from format params #5042
Conversation
…ation Signed-off-by: Heng Qian <[email protected]>
qianheng-aws
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,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplace the Explain-format enum with a new ExplainMode enum and propagate explain-mode handling across AST, plan creation, execution engine, PPL request/transport, plugin, tests, and integration test expected outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST_Plugin
participant TransportLayer
participant PPLService
participant AstBuilder
participant ExecutionEngine
participant OpenSearch
Client->>REST_Plugin: HTTP explain request (format, mode)
REST_Plugin->>TransportLayer: build TransportPPLQueryRequest(format, explainMode)
TransportLayer->>PPLService: deliver PPLQueryRequest(with explainMode)
PPLService->>AstBuilder: build AST (context.explainMode)
AstBuilder->>ExecutionEngine: request plan/explain(plan, ExplainMode)
ExecutionEngine->>OpenSearch: execute explain using ExplainMode
OpenSearch-->>ExecutionEngine: explain result
ExecutionEngine-->>REST_Plugin: ExplainResponse (serialized per format)
REST_Plugin-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java (1)
50-57: Fix missingexplainModeinitialization in multipleStatementBuilderContextcallers.The
explainModefield is accessed at line 41 whenisExplain=truebut is not set by several callers:
SQLService.java:99-103(production code)CalcitePPLAbstractTest.java:125UnifiedQueryPlanner.java:81AstStatementBuilderTest.java:75PPLQueryDataAnonymizerTest.java:956Only
PPLService.java:98-102properly sets the field. Update all other builders to include.explainMode(...)or provide a sensible default value.legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java (1)
15-37: Add JavaDoc to public enum and methods.The removal of explain-related formats (SIMPLE, STANDARD, EXTENDED, COST) and the
EXPLAIN_FORMATSmap is correct and aligns with moving explain mode handling elsewhere. The legacyFormatenum now correctly focuses only on response formats (JDBC, CSV, RAW, TABLE), and there are no remaining references in the legacy module to the removed constants.However, the public enum
Format, public methodof(), and public static fieldRESPONSE_FORMATSlack JavaDoc documentation. Per coding guidelines, all public classes and methods must have proper JavaDoc.
🤖 Fix all issues with AI agents
In @integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java:
- Around line 61-65: The deprecation Javadoc for explainQueryToString(String)
incorrectly directs users to explainQueryYaml(String) despite this method
returning JSON; update the deprecation notice to reference the correct
overloaded method explainQueryToString(String, ExplainMode) (or clarify it
returns JSON) so callers are directed to the JSON-capable alternative; modify
the Javadoc line above the method (and the @Deprecated tag comment) to point to
{@link #explainQueryToString(String, ExplainMode)}.
- Around line 79-84: The explainQuery method constructs the EXPLAIN API path
using enum toString() values which yield uppercase names; replace the enum
arguments in the String.format call with the proper lowercase getters (use
Format.getFormatName() and ExplainMode.getModeName()) so
String.format(EXPLAIN_API_ENDPOINT, format.getFormatName(), mode.getModeName())
is used instead of passing format and mode directly; update the call in the
explainQuery method to reference these getters and remove reliance on
toString().
🧹 Nitpick comments (9)
core/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.java (1)
35-35: Consider using a concreteExplainModeenum value instead of mocking.Mocking an enum is unconventional and can lead to unexpected behavior. Since
ExplainModeis likely a simple enum, using a concrete value likeExplainMode.SIMPLEorExplainMode.STANDARDwould be clearer and more idiomatic.Suggested change
- @Mock private ExplainMode mode; + private ExplainMode mode = ExplainMode.SIMPLE;Then remove
@Mockand use the concrete enum value directly in test methods.core/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.java (1)
28-34: Add@param modeto the JavaDoc.Per coding guidelines, public methods must have complete JavaDoc with
@paramfor all parameters. Themodeparameter is undocumented.Suggested JavaDoc update
/** * Explain query execution. * * @param listener query explain response listener. + * @param mode the explain mode determining output detail level. */ public abstract void explain( ResponseListener<ExecutionEngine.ExplainResponse> listener, ExplainMode mode);core/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.java (1)
12-29: Consider catching specific exception and adding JavaDoc.The enum design is clean and the use of
Locale.ROOTfor case-insensitive matching is good practice. Two suggestions:
Catch specific exception:
valueOf()throwsIllegalArgumentExceptionfor invalid names. Catching the genericExceptionis overly broad and could mask unexpected errors.Missing JavaDoc: Per coding guidelines, public classes and methods in
core/src/main/javashould have JavaDoc with@param,@returndocumentation.Suggested improvements
+/** + * Defines the explain output mode for query plan visualization. + */ @RequiredArgsConstructor public enum ExplainMode { SIMPLE("simple"), STANDARD("standard"), EXTENDED("extended"), COST("cost"); @Getter private final String modeName; + /** + * Converts a string mode name to an ExplainMode enum value. + * + * @param mode the mode name (case-insensitive), may be null + * @return the corresponding ExplainMode, defaults to STANDARD if null, empty, or invalid + */ public static ExplainMode of(String mode) { if (mode == null || mode.isEmpty()) return STANDARD; try { return ExplainMode.valueOf(mode.toUpperCase(Locale.ROOT)); - } catch (Exception e) { + } catch (IllegalArgumentException e) { return ExplainMode.STANDARD; } } }core/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.java (1)
22-33: Consider adding JavaDoc to the constructor.As per coding guidelines for
core/src/main/java/**/*.java, public methods should have JavaDoc with@param,@return, and@throwsannotations. The constructor currently has a minimal comment.📝 Suggested JavaDoc
- /** Constructor. */ + /** + * Constructs an ExplainPlan that wraps another plan for explanation. + * + * @param queryId the unique query identifier + * @param queryType the type of query (SQL or PPL) + * @param plan the underlying plan to explain + * @param mode the explain mode (SIMPLE, STANDARD, EXTENDED, or COST) + * @param explainListener listener for the explain response + */ public ExplainPlan(legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.java (1)
42-54: Remove unusedpathparameter.The
pathparameter is not used in the method body and all callers invoke the single-parameter overload that passesnull. Simplify by removing this unused parameter.♻️ Suggested cleanup
- public static Format getFormat(Map<String, String> requestParams, String path) { + public static Format getFormat(Map<String, String> requestParams) { String formatName = requestParams.containsKey(QUERY_PARAMS_FORMAT) ? requestParams.get(QUERY_PARAMS_FORMAT).toLowerCase() : DEFAULT_RESPONSE_FORMAT; Optional<Format> optionalFormat = Format.of(formatName);Also remove the overload method
getFormat(Map<String, String> requestParams)that currently just delegates to this method withnull.ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (1)
93-95: Missing JavaDoc on public method.Per coding guidelines, public methods should have JavaDoc with
@returndocumentation.📝 Suggested JavaDoc
+ /** + * Convert the explain mode string to an ExplainMode instance. + * + * @return the ExplainMode corresponding to the explainMode string + */ public ExplainMode mode() { return ExplainMode.of(explainMode); }plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (1)
137-144: Add JavaDoc for new private method.While private methods don't strictly require JavaDoc, adding a brief comment explaining the null return for non-explain requests would improve maintainability.
📝 Suggested documentation
+ /** + * Get explain mode from request parameters. + * + * @return the explain mode string, or null if not an explain request + */ private static String getExplainMode(Map<String, String> requestParams, String path) { if (!isExplainRequest(path)) { return null; } return requestParams .getOrDefault(QUERY_PARAMS_EXPLAIN_MODE, DEFAULT_EXPLAIN_MODE) .toLowerCase(); }core/src/main/java/org/opensearch/sql/executor/QueryService.java (2)
204-207: Minor style: redundant parentheses.The parentheses around
(mode != ExplainMode.STANDARD)are unnecessary.📝 Suggested simplification
- if (mode != null && (mode != ExplainMode.STANDARD)) { + if (mode != null && mode != ExplainMode.STANDARD) { throw new UnsupportedOperationException( "Explain mode " + mode.name() + " is not supported in v2 engine"); }
188-202: Missing@paramformodeparameter in JavaDoc.The JavaDoc should document the new
modeparameter.📝 Suggested JavaDoc update
/** * Explain the query in {@link UnresolvedPlan} using {@link ResponseListener} to get and format * explain response. * * @param plan {@link UnresolvedPlan} * @param queryType {@link QueryType} * @param listener {@link ResponseListener} for explain response + * @param mode {@link ExplainMode} for explain output format (only STANDARD supported in v2) * @param calciteFailure Optional failure thrown from calcite */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamllegacy/src/main/java/org/opensearch/sql/legacy/executor/Format.javalegacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javaplugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/PPLService.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javaprotocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java
💤 Files with no reviewable changes (2)
- integ-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.json
- integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.json
🧰 Additional context used
📓 Path-based instructions (10)
**/*.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:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javalegacy/src/main/java/org/opensearch/sql/legacy/executor/Format.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javalegacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javappl/src/main/java/org/opensearch/sql/ppl/PPLService.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.javaplugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javalegacy/src/main/java/org/opensearch/sql/legacy/executor/Format.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javalegacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequestParam.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javaprotocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javappl/src/main/java/org/opensearch/sql/ppl/PPLService.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javaplugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javaopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.javaplugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javaplugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/util/MatcherUtils.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
integ-test/src/test/resources/**/*
⚙️ CodeRabbit configuration file
integ-test/src/test/resources/**/*: - Verify test data is realistic and representative
- Check data format matches expected schema
- Ensure test data covers edge cases and boundary conditions
Files:
integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.json
core/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/**/*.java: - New functions MUST have unit tests in the same commit
Files:
core/src/main/java/org/opensearch/sql/executor/execution/AbstractPlan.javacore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.javacore/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.javacore/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.java
core/src/main/java/org/opensearch/sql/ast/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/org/opensearch/sql/ast/**/*.java: - AST nodes must be immutable where possible
- Follow visitor pattern for AST traversal
- Ensure proper toString() implementation for debugging
Files:
core/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.java
**/ppl/**/*.java
⚙️ CodeRabbit configuration file
**/ppl/**/*.java: - For PPL parser changes, verify grammar tests with positive/negative cases
- Check AST generation for new syntax
- Ensure corresponding AST builder classes are updated
- Validate edge cases and boundary conditions
Files:
ppl/src/main/java/org/opensearch/sql/ppl/PPLService.javappl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.javappl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧠 Learnings (8)
📚 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:
integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.jsoncore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javainteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yamlcore/src/main/java/org/opensearch/sql/executor/QueryService.javainteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_cost.jsonopensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.javacore/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.javainteg-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-11T05:27:39.856Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 0
File: :0-0
Timestamp: 2025-12-11T05:27:39.856Z
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:
integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yaml
📚 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:
integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlcore/src/main/java/org/opensearch/sql/executor/ExecutionEngine.javacore/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.javacore/src/main/java/org/opensearch/sql/executor/QueryService.javacore/src/main/java/org/opensearch/sql/ast/statement/Explain.java
📚 Learning: 2025-12-29T05:32:11.893Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:11.893Z
Learning: In opensearch-project/sql, when creating custom Calcite RelNode classes that extend EnumerableLimitSort or other Calcite RelNode types, always override the `copy` method. Without overriding copy, the class will downgrade to its parent class type during copy operations, losing the custom implementation.
Applied to files:
integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yamlinteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.jsoninteg-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yaml
📚 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:
integ-test/src/test/resources/expectedOutput/calcite/explain_output_extended.json
📚 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: Update corresponding AST builder classes when making PPL grammar changes
Applied to files:
ppl/src/main/java/org/opensearch/sql/ppl/PPLService.javappl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.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:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧬 Code graph analysis (2)
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
Getter(14-36)
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (1)
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
Getter(14-36)
⏰ 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). (29)
- GitHub Check: security-it-linux (25)
- GitHub Check: security-it-linux (21)
- GitHub Check: build-linux (25, unit)
- GitHub Check: build-linux (21, integration)
- GitHub Check: build-linux (21, unit)
- GitHub Check: bwc-tests-full-restart (25)
- GitHub Check: build-linux (21, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (25, integration)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- 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, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (45)
integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java (1)
487-488: LGTM!The addition of ID normalization in
cleanUpYamlfollows the established pattern for handling dynamic values in test output. The regexid = \\d+correctly matches numeric IDs with the expected spacing and normalizes them toid = *for stable test comparisons across different explain modes.integ-test/src/test/resources/expectedOutput/calcite/explain_skip_script_encoding.yaml (1)
1-8: LGTM! Test resource correctly captures expected explain output.The YAML structure is well-formed with consistent logical and physical plan representations:
- Logical plan accurately reflects the query semantics (filter on address equality and arithmetic age condition)
- Physical plan shows proper pushdown context with PROJECT, SCRIPT, and LIMIT operations
- Script parameters (DIGESTS/SOURCES) correctly map to the filter operands
- Hardcoded
utcTimestamp: 0ensures deterministic test outputThe test data is realistic and covers meaningful edge cases including string comparison, arithmetic expressions in filters, and multi-field projection pushdown.
integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.yaml (1)
1-14: LGTM!The YAML structure is valid and the logical plan hierarchy appropriately represents a simple explain output for Calcite, showing only node names without detailed attributes. The plan structure (SystemLimit → Project → Filter → Sort → Aggregate → Filter → IndexScan) is realistic for a typical query.
integ-test/src/test/resources/expectedOutput/calcite/explain_extended_for_standardization.yaml (1)
1-9: LGTM!The extended explain output correctly includes both logical and physical plan sections with appropriate detail levels. The logical section shows typed attributes (fetch, type, expressions), and the physical section includes the full
PushDownContextandOpenSearchRequestBuilderdetails expected for extended mode verification.protocol/src/test/java/org/opensearch/sql/protocol/response/format/FormatTest.java (1)
53-58: LGTM!The test correctly reflects the updated default behavior where
Format.ofExplain("")now returnsFormat.JSONinstead of the previousFormat.STANDARD. This aligns with the PR's separation of explain mode from format parameters, where format controls output serialization (JSON/YAML) and mode controls verbosity level.integ-test/src/test/resources/expectedOutput/calcite/explain_output_simple.json (1)
1-5: LGTM!The JSON test resource correctly mirrors the YAML version with identical logical plan content, validating that the same simple explain mode produces consistent output across different formats.
ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java (1)
152-157: LGTM!The change from
node.getFormat()tonode.getMode()correctly aligns the anonymizer with the newExplainModeAPI. The output will now reflect the explain mode (e.g., "explain simple", "explain extended") in anonymized queries.plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java (1)
77-82: LGTM!Adding "mode" to the response parameters correctly enables the explain mode query parameter to be processed in PPL REST requests, supporting the new separation of mode from format.
ppl/src/main/java/org/opensearch/sql/ppl/PPLService.java (1)
98-102: LGTM!The addition of
.explainMode(request.getExplainMode())correctly propagates the explain mode through the AST building context, following the established builder pattern. WhilegetExplainMode()can return null when not an explain request, this is safely handled downstream: theExplainclass constructor callsExplainMode.of(mode), which defaults toSTANDARDwhen null is passed.core/src/test/java/org/opensearch/sql/executor/execution/ExplainPlanTest.java (2)
37-45: Test correctly verifies the explain execution flow.The test properly validates that
ExplainPlan.execute()delegates toqueryPlan.explain()with the correct listener and mode parameters.
47-58: Exception test validates unsupported explain-on-explain scenario.The test correctly verifies that calling
explain()on anExplainPlanthrowsUnsupportedOperationExceptionwith the expected message.integ-test/src/test/resources/expectedOutput/calcite/explain_output_standard.yaml (1)
1-21: Test resource correctly defines expected STANDARD mode explain output.The YAML structure properly separates logical and physical plans for the Calcite explain output. The test data is realistic and aligns with the mode-based explain testing infrastructure introduced in this PR.
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java (1)
40-42: Correctly separates explain mode from format in Explain construction.The change properly uses
context.explainModeinstead ofcontext.formatwhen building theExplainstatement, achieving the PR's objective of separating explain mode from format parameters.integ-test/src/test/resources/expectedOutput/calcite/explain_output_standard.json (1)
1-6: Test resource correctly defines expected STANDARD mode explain output in JSON format.The JSON structure is valid and content is consistent with the corresponding YAML resource. This enables testing the same explain mode across different output formats.
integ-test/src/test/resources/expectedOutput/calcite/explain_output_extended.json (1)
1-7: LGTM - Extended explain output test resource.The test resource correctly captures all three sections (logical, physical, extended) expected from the extended explain mode. The extended section appropriately includes the Java-based bind implementation and iterator logic that Calcite generates.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (4)
28-29: LGTM - Import changes for ExplainMode usage.The imports are correctly added for the new
ExplainModeenum andSettings.Keyused in the test methods.
381-389: LGTM - Updated to use ExplainMode with explicit format.The test correctly separates the explain mode (EXTENDED) from the format (YAML), demonstrating the new API pattern introduced in this PR.
2209-2219: LGTM - Consistent use of ExplainMode.EXTENDED.The test correctly uses the new
ExplainMode.EXTENDEDparameter and loads the appropriate expected output file.
2322-2347: Good test coverage for all explain modes and formats.This test comprehensively validates the separation of explain mode from format by iterating through all
ExplainModevalues (SIMPLE, STANDARD, EXTENDED, COST) and testing both YAML and JSON output formats for each mode. The helper methodsexplainQueryYaml(query, ExplainMode)andexplainQueryToString(query, ExplainMode)are available in the parentExplainITclass, and corresponding test resource files exist atinteg-test/src/test/resources/expectedOutput/calcite/.integ-test/src/test/resources/expectedOutput/calcite/explain_output_cost.json (1)
1-6: LGTM - Cost explain output test resource.The test resource correctly includes per-operator cost statistics (rowcount, cumulative cost, cpu, io, id) in both logical and physical plan sections, which is the expected format for
ExplainMode.COST. The absence of an "extended" section is appropriate for the cost mode.protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java (2)
54-57: Default explain format changed from "standard" to "json".The default format is now
"json"instead of"standard". This makes sense since the Format enum now represents output formats (JSON/YAML), not explain modes.
21-26: Clean separation of format from mode - refactoring verified as complete.The refactoring correctly removes explain modes (SIMPLE, STANDARD, EXTENDED, COST) from the Format enum, leaving only output formats (JSON, YAML). This aligns with the PR objective of separating explain mode from format parameters.
No external usages of the removed Format constants exist in the codebase—all references have been migrated to the ExplainMode enum. The default explain format has been updated to "json" as intended.
core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java (2)
16-16: LGTM - Import updated to ExplainMode.The import correctly references the new
ExplainModeclass that replacesExplain.ExplainFormat.
54-58: Verify implementation consistency across all ExecutionEngine implementations.The interface method correctly uses
ExplainModeas a parameter type (lines 54-58). However, manual verification is needed to ensure all implementing classes have been updated consistently. The proposed shell script should be executed to identify all classes implementingExecutionEngineand cross-check theirexplain()method signatures to confirm they all reflect the same parameter type change.core/src/main/java/org/opensearch/sql/executor/execution/CommandPlan.java (1)
8-8: LGTM! Clean migration to ExplainMode.The import and method signature updates are consistent with the new
ExplainModeabstraction. The behavior (throwingUnsupportedOperationException) is appropriately preserved.Minor note: Per coding guidelines, public methods should have JavaDoc with
@paramand@throwsannotations. Consider adding documentation to theexplainmethod.Also applies to: 48-52
core/src/main/java/org/opensearch/sql/executor/execution/QueryPlan.java (1)
10-10: LGTM! Correct migration to ExplainMode.The import update, method signature change, and forwarding of
modetoqueryService.explain()are all consistent with the refactoring. The pagination guard and error handling remain correctly in place.Also applies to: 70-80
integ-test/src/test/resources/expectedOutput/calcite/explain_output_cost.yaml (1)
1-21: Good test coverage for COST explain mode.The expected output file provides comprehensive coverage including:
- Logical plan with cost metrics (rowcount, cumulative cost with cpu/io)
- Physical plan with enumerable operators
- Complex query pattern (aggregation + filter + sort + window function + limit)
This aligns well with testing the Calcite explain cost mode. Based on learnings, this properly tests SQL generation and optimization paths for Calcite integration.
core/src/main/java/org/opensearch/sql/ast/statement/Explain.java (1)
18-30: Clean refactoring to ExplainMode.The changes correctly:
- Replace
ExplainFormatfield withExplainMode- Delegate parsing to
ExplainMode.of(mode)which handles null/empty/invalid inputs- Maintain immutability with
finalfield (appropriate for AST nodes per guidelines)- Preserve the two-constructor pattern for default and explicit mode specification
The null-safe chaining through the overloaded constructor to
ExplainMode.of(null)→STANDARDis a clean default behavior.opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java (1)
39-39: LGTM - Clean migration to ExplainMode.The transition from
ExplainFormattoExplainModeis implemented correctly. The conditional logic properly maps each mode to the appropriateSqlExplainLeveland the control flow for SIMPLE, COST, and EXTENDED modes is maintained.Also applies to: 166-205
core/src/main/java/org/opensearch/sql/executor/execution/QueryPlanFactory.java (2)
77-83: LGTM - Correct migration to ExplainMode.The cursor-based explain path correctly uses
ExplainMode.of(format)to convert the format string to the new mode type.
129-143: LGTM - Consistent with Explain AST node changes.The
visitExplainmethod correctly retrieves the mode from the Explain node vianode.getMode(), which aligns with the updatedExplainclass that now storesExplainMode modeinstead of format.integ-test/src/test/resources/expectedOutput/calcite/explain_output_extended.yaml (1)
1-167: LGTM - Valid test resource for extended explain mode.This test resource correctly captures the expected output structure for the extended explain mode, including:
- Logical plan with Calcite operators
- Physical plan with enumerable operators
- Extended section with generated Java binding code
The content follows expected Calcite Linq4j patterns. As per coding guidelines, the test data appears representative of the explain output schema.
core/src/main/java/org/opensearch/sql/executor/execution/ExplainPlan.java (1)
14-44: LGTM - Clean type migration.The transition from
Explain.ExplainFormattoExplainModeis correctly implemented across the field, constructor, and method signatures.plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryRequest.java (3)
66-78: LGTM - Correct serialization of explainMode.The
explainModefield is properly serialized and deserialized usingwriteOptionalString/readOptionalString, maintaining backward compatibility with null values.Also applies to: 99-110
141-148: LGTM - Correct propagation of explainMode.The
toPPLQueryRequestmethod correctly passes theexplainModeparameter to the newPPLQueryRequestconstructor signature, enabling the mode to be propagated through the request lifecycle.
37-37: No action needed. The null safety concern is unfounded.The code
pplQueryRequest.mode().getModeName()is safe becauseExplainMode.of()always returns a non-null enum constant (either the matched mode orSTANDARDas a fallback), never null. ThegetModeName()getter on the enum will always return a valid string.Likely an incorrect or invalid review comment.
ppl/src/main/java/org/opensearch/sql/ppl/domain/PPLQueryRequest.java (3)
14-14: LGTM: New ExplainMode import and field.The import and field addition properly support the new explain mode feature.
Also applies to: 28-28
49-67: LGTM: Constructor chaining with explainMode parameter.The constructor properly threads the
explainModethrough, defaulting toSTANDARDfor the 4-argument form.
93-95: No issue found with null handling.The
ExplainMode.of()method already explicitly handles null inputs by returningExplainMode.STANDARDon line 22 (if (mode == null || mode.isEmpty()) return STANDARD;). Themode()method is safe and will not throw an NPE for non-explain requests.Likely an incorrect or invalid review comment.
integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java (1)
29-29: LGTM: Updated imports and endpoint template.The new
EXPLAIN_API_ENDPOINTtemplate with format and mode parameters consolidates the previous separate endpoints.Also applies to: 33-33, 38-38
plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.java (2)
22-26: LGTM: New constants for explain mode support.The constants are appropriately named and follow the existing pattern for query parameters.
62-62: LGTM: Clean integration of explainMode in request parsing.The
explainModeis extracted and passed toPPLQueryRequestfollowing the same pattern as other parameters.Also applies to: 76-76
core/src/main/java/org/opensearch/sql/executor/QueryService.java (3)
29-29: LGTM: Import updated to ExplainMode.The import change aligns with the refactoring from
ExplainFormattoExplainMode.
79-90: LGTM: Updated explain method signature.The method cleanly delegates to Calcite or legacy paths while propagating the
ExplainMode.
133-167: LGTM: Calcite explain path updated with mode.The
ExplainModeis correctly propagated to the execution engine and through the fallback path.
| /** Deprecated, use {@link #explainQueryYaml(String)} */ | ||
| @Deprecated | ||
| protected String explainQueryToString(String query) throws IOException { | ||
| return explainQueryToString(query, false); | ||
| return explainQueryToString(query, ExplainMode.STANDARD); | ||
| } |
There was a problem hiding this comment.
Inconsistent deprecation notice.
The @Deprecated comment suggests using explainQueryYaml(String), but the deprecated method explainQueryToString(String) returns JSON format. Consider updating the deprecation notice to point to explainQueryToString(String, ExplainMode) instead.
📝 Suggested fix
- /** Deprecated, use {@link #explainQueryYaml(String)} */
+ /** Deprecated, use {@link #explainQueryToString(String, ExplainMode)} */
@Deprecated
protected String explainQueryToString(String query) throws IOException {
return explainQueryToString(query, ExplainMode.STANDARD);
}📝 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.
| /** Deprecated, use {@link #explainQueryYaml(String)} */ | |
| @Deprecated | |
| protected String explainQueryToString(String query) throws IOException { | |
| return explainQueryToString(query, false); | |
| return explainQueryToString(query, ExplainMode.STANDARD); | |
| } | |
| /** Deprecated, use {@link #explainQueryToString(String, ExplainMode)} */ | |
| @Deprecated | |
| protected String explainQueryToString(String query) throws IOException { | |
| return explainQueryToString(query, ExplainMode.STANDARD); | |
| } |
🤖 Prompt for AI Agents
In @integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java around
lines 61 - 65, The deprecation Javadoc for explainQueryToString(String)
incorrectly directs users to explainQueryYaml(String) despite this method
returning JSON; update the deprecation notice to reference the correct
overloaded method explainQueryToString(String, ExplainMode) (or clarify it
returns JSON) so callers are directed to the JSON-capable alternative; modify
the Javadoc line above the method (and the @Deprecated tag comment) to point to
{@link #explainQueryToString(String, ExplainMode)}.
| private String explainQuery(String query, Format format, ExplainMode mode) throws IOException { | ||
| Response response = | ||
| client() | ||
| .performRequest( | ||
| buildRequest( | ||
| query, extended ? EXTENDED_EXPLAIN_API_ENDPOINT : EXPLAIN_API_ENDPOINT)); | ||
| .performRequest(buildRequest(query, String.format(EXPLAIN_API_ENDPOINT, format, mode))); | ||
| Assert.assertEquals(200, response.getStatusLine().getStatusCode()); | ||
| String responseBody = getResponseBody(response, true); | ||
| return responseBody.replace("\\r\\n", "\\n"); | ||
| return getResponseBody(response, true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Format enum definition
echo "=== Format enum definition ==="
rg "enum Format" --type=java -B 2 -A 20
echo ""
echo "=== ExplainMode enum definition ==="
rg "enum ExplainMode" --type=java -B 2 -A 20
echo ""
echo "=== EXPLAIN_API_ENDPOINT definition ==="
rg "EXPLAIN_API_ENDPOINT" --type=java -B 2 -A 2
echo ""
echo "=== Check PPLQueryRequestFactory for reference ==="
rg "PPLQueryRequestFactory" --type=java -lRepository: opensearch-project/sql
Length of output: 15965
🏁 Script executed:
#!/bin/bash
# Get complete Format enum definition
echo "=== Complete Format enum ==="
cat -n protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java | head -50
echo ""
echo "=== Complete ExplainMode enum ==="
cat -n core/src/main/java/org/opensearch/sql/ast/statement/ExplainMode.javaRepository: opensearch-project/sql
Length of output: 2713
🏁 Script executed:
#!/bin/bash
# Check if there are tests that validate this endpoint behavior
echo "=== Check test execution results or other usages ==="
rg "explainQuery" integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java -B 2 -A 10
echo ""
echo "=== Check REST handler that processes the endpoint ==="
rg "format.*mode" plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java -B 3 -A 3Repository: opensearch-project/sql
Length of output: 2186
🏁 Script executed:
#!/bin/bash
# Check RestPPLQueryAction implementation for parameter parsing
echo "=== RestPPLQueryAction request parameter handling ==="
cat -n plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java | grep -A 100 "protected RestChannelConsumer"
echo ""
echo "=== Check PPLQueryRequestFactory for parameter handling ==="
cat -n plugin/src/main/java/org/opensearch/sql/plugin/request/PPLQueryRequestFactory.javaRepository: opensearch-project/sql
Length of output: 9669
🏁 Script executed:
#!/bin/bash
# Verify the Format.ofExplain() and Format.of() methods can handle uppercase
echo "=== Check Format.ofExplain() and Format.of() implementations ==="
cat -n protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java | tail -30Repository: opensearch-project/sql
Length of output: 1397
Use enum getter methods instead of relying on toString().
Line 82 calls String.format(EXPLAIN_API_ENDPOINT, format, mode), which invokes the default toString() methods on Format and ExplainMode enums. Neither enum overrides toString(), so it returns the uppercase constant names (e.g., "JSON", "YAML" for Format; "STANDARD", "SIMPLE" for ExplainMode) rather than the expected lowercase parameter values.
While the server-side code compensates by calling .toLowerCase() in PPLQueryRequestFactory, this design is fragile and masks the issue. Use format.getFormatName() and mode.getModeName() instead, which directly return the correct lowercase values.
🤖 Prompt for AI Agents
In @integ-test/src/test/java/org/opensearch/sql/ppl/PPLIntegTestCase.java around
lines 79 - 84, The explainQuery method constructs the EXPLAIN API path using
enum toString() values which yield uppercase names; replace the enum arguments
in the String.format call with the proper lowercase getters (use
Format.getFormatName() and ExplainMode.getModeName()) so
String.format(EXPLAIN_API_ENDPOINT, format.getFormatName(), mode.getModeName())
is used instead of passing format and mode directly; update the call in the
explainQuery method to reference these getters and remove reliance on
toString().
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java (1)
62-62: Consider adding test coverage for other ExplainMode values.The PR introduces multiple explain modes (SIMPLE, STANDARD, EXTENDED, COST), but tests only exercise
STANDARD. Consider parameterizing the explain tests to cover different modes, ensuring theQueryService.explain()method correctly handles all supported values.♻️ Example using JUnit 5 parameterized test
@ParameterizedTest @EnumSource(ExplainMode.class) public void testExplainWithDifferentModes(ExplainMode mode) { queryService().explainSuccess().handledByExplainOnResponse(mode); }This would require adjusting
handledByExplainOnResponseto accept a mode parameter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javadocs/user/ppl/interfaces/endpoint.mdinteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
✅ Files skipped from review due to trivial changes (1)
- docs/user/ppl/interfaces/endpoint.md
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
integ-test/**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
End-to-end scenarios need integration tests in
integ-test/module
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
⚙️ CodeRabbit configuration file
integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources
- Verify test data files exist in integ-test/src/test/resources/
- Check test assertions are meaningful and specific
- Validate tests clean up resources after execution
- Ensure tests are independent and can run in any order
- Flag tests that reference non-existent indices (e.g., EMP)
- Verify integration tests are in correct module (integ-test/)
- Check tests can be run with ./gradlew :integ-test:integTest
- Ensure proper test data setup and teardown
- Validate end-to-end scenario coverage
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/*IT.java
📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)
Name integration tests with
*IT.javasuffix in OpenSearch SQL
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
**/calcite/**/*.java
⚙️ CodeRabbit configuration file
**/calcite/**/*.java: - Follow existing Calcite integration patterns
- Verify RelNode visitor implementations are complete
- Check RexNode handling follows project conventions
- Validate SQL generation is correct and optimized
- Ensure Calcite version compatibility
- Follow existing patterns in CalciteRelNodeVisitor and CalciteRexNodeVisitor
- Document any Calcite-specific workarounds
- Test compatibility with Calcite version constraints
Files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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:
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.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:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
📚 Learning: 2025-12-29T05:32:03.491Z
Learnt from: LantaoJin
Repo: opensearch-project/sql PR: 4993
File: opensearch/src/main/java/org/opensearch/sql/opensearch/planner/physical/CalciteEnumerableTopK.java:20-20
Timestamp: 2025-12-29T05:32:03.491Z
Learning: For any custom Calcite RelNode class (e.g., ones that extend EnumerableLimitSort or other Calcite RelNode types), always override the copy method. If copy is not overridden, cloning/copy operations may downgrade the instance to the parent class type, losing the custom behavior. In your implementation, ensure copy returns a new instance of the concrete class with all relevant fields and traits preserved, mirroring the current instance state.
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java
🧬 Code graph analysis (1)
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.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). (11)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (8)
core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java (3)
23-23: LGTM!Import correctly updated to use the new
ExplainModeenum, aligning with the PR's objective to separate explain mode from format parameters.
210-226: LGTM!The method correctly passes the
modeparameter toqueryService.explain(), and the response listener properly validates the successful explain response.
228-244: LGTM!The failure path correctly uses the
modeparameter and properly validates thatIllegalStateExceptionis caught by the failure handler.integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java (5)
28-29: LGTM!The new imports properly support the PR's objective of separating explain mode from format. The
ExplainModeenum replaces the previous format-based explain handling, and theKeyimport simplifies the settings references.
1723-1735: LGTM!The simplified
Key.PPL_SYNTAX_LEGACY_PREFERREDreference is cleaner and consistent with the new static import.
2209-2219: LGTM!The test now explicitly specifies
ExplainMode.EXTENDEDrather than relying on implicit format-based mode selection. This makes the test intent clearer and aligns with the PR's separation of concerns.
381-389: LGTM! Demonstrates the new mode/format separation.The test correctly uses
explainQueryYaml(query, ExplainMode.EXTENDED)to specify both the output format (YAML) and explain verbosity (EXTENDED) independently, aligning with the PR objective. The expected YAML resource file exists in the resources directory.
2322-2348: Good comprehensive test coverage for the new mode/format API.This test effectively validates the separation of explain mode from format by testing all four
ExplainModevalues (SIMPLE, STANDARD, EXTENDED, COST) with both YAML and JSON output formats. The query covers a good variety of PPL commands, and all 8 expected resource files are present ininteg-test/src/test/resources/expectedOutput/calcite/.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java (1)
112-128: LGTM! Error handling test correctly updated for new parameter.The test properly validates that explain with pagination throws
NotImplementedExceptionand now correctly passes themodeparameter.💡 Optional: Consider using assertInstanceOf (JUnit 5.8+)
If the project uses JUnit 5.8+,
assertInstanceOfprovides better error messages:- assertTrue(e instanceof NotImplementedException); + assertInstanceOf(NotImplementedException.class, e);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.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). (17)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- 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, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- 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: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: Update draft release notes
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: test-sql-cli-integration (21)
- GitHub Check: enforce-label
🔇 Additional comments (2)
core/src/test/java/org/opensearch/sql/executor/execution/QueryPlanTest.java (2)
23-23: LGTM! Import and mock field correctly updated to use new ExplainMode type.The migration from
Explain.ExplainFormattoExplainModeis consistent with the PR objective of separating explain verbosity from response format.Also applies to: 48-48
58-64: LGTM! Test properly updated to verify new ExplainMode parameter.The test correctly verifies that
QueryService.explain()is invoked with the newmodeparameter, and the verification matches the expected method signature.
Signed-off-by: Heng Qian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java (1)
66-78: Consider using an actual enum value instead of mocking.The API update from
ExplainFormattoExplainModeis correctly applied. However, mocking an enum is generally fragile and unnecessary here—since the test only verifies thatexplain()throws regardless of the mode value, using an actual enum constant would be simpler and more robust.♻️ Suggested refactor
- ExplainMode mode = mock(ExplainMode.class); + ExplainMode mode = ExplainMode.STANDARD;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
⚙️ CodeRabbit configuration file
**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring
- Flag classes >500 lines as needing organization review
- Check for dead code, unused imports, and unused variables
- Identify code reuse opportunities across similar implementations
- Assess holistic maintainability - is code easy to understand and modify?
- Flag code that appears AI-generated without sufficient human review
- Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)
- Check for proper JavaDoc on public classes and methods
- Flag redundant comments that restate obvious code
- Ensure proper error handling with specific exception types
- Check for Optional usage instead of null returns
- Validate proper use of try-with-resources for resource management
Files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java
**/test/**/*.java
⚙️ CodeRabbit configuration file
**/test/**/*.java: - Verify NULL input tests for all new functions
- Check boundary condition tests (min/max values, empty inputs)
- Validate error condition tests (invalid inputs, exceptions)
- Ensure multi-document tests for per-document operations
- Flag smoke tests without meaningful assertions
- Check test naming follows pattern: test
- Verify test data is realistic and covers edge cases
- Verify test coverage for new business logic
- Ensure tests are independent and don't rely on execution order
- Validate meaningful test data that reflects real-world scenarios
- Check for proper cleanup of test resources
Files:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.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:
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.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, doc)
- GitHub Check: build-linux (25, doc)
- GitHub Check: build-linux (21, unit)
- GitHub Check: build-linux (25, integration)
- GitHub Check: build-linux (21, integration)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- GitHub Check: build-linux (25, unit)
- GitHub Check: bwc-tests-rolling-upgrade (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-full-restart (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: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: build-windows-macos (macos-14, 25, integration)
- GitHub Check: build-windows-macos (macos-14, 25, doc)
- GitHub Check: build-windows-macos (macos-14, 21, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, integration)
- GitHub Check: build-windows-macos (windows-latest, 21, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: build-windows-macos (macos-14, 21, doc)
- 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: security-it-windows-macos (macos-14, 25)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (1)
core/src/test/java/org/opensearch/sql/executor/execution/CommandPlanTest.java (1)
22-22: LGTM!Import correctly updated to reflect the new
ExplainModetype replacing the oldExplainFormat.
|
|
||
| To translate your query, send it to explain endpoint. The explain output is OpenSearch domain specific language (DSL) in JSON format. You can just copy and paste it to your console to run it against OpenSearch directly. | ||
| Explain output could be set different formats: `standard` (the default format), `simple`, `extended`, `dsl`. | ||
| Explain API supports various mode: `standard` (the default format), `simple`, `extended`, `cost`. |
There was a problem hiding this comment.
+1, mode is more clear then format.
Q, PPL should support format=[simple, extended, cost] until 4.x, then we can deprecated it, right? If yes, we should add deprecated warnning info in resposne.
2 participants
Description
Introduce a new query parameter
mode(allowsSIMPLE,STANDARD,EXTENDED,COST) specifically for controlling the explain verbosity, while keeping format strictly for output serialization format(allowsJSON,YAMLfor explain API or command).e.g.
Before this PR, when setting
format=yaml, users cannot specify the explain mode anymore with the explain mode bounded to the defaultSTANDARD.Related Issues
Resolves #4940
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.