Add OUTPUT as an alias for REPLACE in Lookup#5049
Conversation
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Swiddis
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughExtended PPL LOOKUP to accept the OUTPUT keyword as a synonym for REPLACE (SPL compatibility); updated grammar, parser comment, docs, and added unit and integration tests covering OUTPUT usage and aliasing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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: 1
🤖 Fix all issues with AI agents
In `@docs/user/ppl/cmd/lookup.md`:
- Line 39: The table cell containing the parameter name "(replace | append |
output)" triggers MD056 because the pipe characters are being parsed as column
separators; update the parameter token to escape each pipe (e.g., change
"(replace | append | output)" to "(replace \| append \| output)" inside the
backticks) so the pipes are treated as literal characters and the table renders
correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/user/ppl/cmd/lookup.mdinteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLLookupIT.javappl/src/main/antlr/OpenSearchPPLParser.g4ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.java
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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:
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLLookupIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.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:
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.javainteg-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLLookupIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.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/parser/AstBuilder.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.java
**/*.g4
⚙️ CodeRabbit configuration file
**/*.g4: - Check if modifying unrelated grammar files (scope creep)
- Verify grammar rule placement follows project patterns
- Question if new rule needed vs reusing existing rules
- Ensure changes are relevant to the PR's stated purpose
Files:
ppl/src/main/antlr/OpenSearchPPLParser.g4
ppl/src/main/antlr/OpenSearchPPLParser.g4
⚙️ CodeRabbit configuration file
ppl/src/main/antlr/OpenSearchPPLParser.g4: - Identify code reuse opportunities with existing commands
- Validate command follows PPL naming and structure patterns
- Check if command should be alias vs separate implementation
Files:
ppl/src/main/antlr/OpenSearchPPLParser.g4
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/CalcitePPLLookupIT.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/CalcitePPLLookupIT.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/CalcitePPLLookupIT.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:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLLookupIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.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/CalcitePPLLookupIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.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:
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.java
🧠 Learnings (4)
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Update corresponding AST builder classes when making PPL grammar changes
Applied to files:
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java
📚 Learning: 2025-12-02T17:27:55.938Z
Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Test SQL generation and optimization paths for Calcite integration changes
Applied to files:
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLLookupIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.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/CalcitePPLLookupIT.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/CalcitePPLLookupIT.javappl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.java
🪛 markdownlint-cli2 (0.18.1)
docs/user/ppl/cmd/lookup.md
39-39: Table column count
Expected: 3; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
⏰ 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 (21, unit)
- 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-full-restart (25)
- GitHub Check: bwc-tests-full-restart (21)
- GitHub Check: bwc-tests-rolling-upgrade (21)
- 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, 21, -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, 25, integration)
- GitHub Check: build-windows-macos (windows-latest, 25, -PbuildPlatform=windows, integration)
- 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, 25, -PbuildPlatform=windows, unit)
- GitHub Check: build-windows-macos (macos-14, 21, unit)
- GitHub Check: security-it-windows-macos (windows-latest, 25)
- GitHub Check: security-it-windows-macos (windows-latest, 21)
- GitHub Check: build-windows-macos (macos-14, 25, unit)
- GitHub Check: security-it-windows-macos (macos-14, 25)
- GitHub Check: security-it-windows-macos (macos-14, 21)
- GitHub Check: CodeQL-Scan (java)
- GitHub Check: test-sql-cli-integration (21)
🔇 Additional comments (7)
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java (1)
975-977: LGTM!The comment clearly documents that OUTPUT and REPLACE are synonyms. The existing logic correctly handles the new OUTPUT keyword by falling through to
Lookup.OutputStrategy.REPLACEwhen neither APPEND nor explicit REPLACE is specified. This aligns with the grammar change and SPL compatibility goals. Based on learnings, the AST builder is properly updated to reflect the grammar change.ppl/src/main/antlr/OpenSearchPPLParser.g4 (1)
479-479: LGTM!The grammar change is minimal and correctly placed. Adding
OUTPUTto the existing alternatives(APPEND | REPLACE | OUTPUT)follows project patterns and reuses the existingOUTPUTtoken. As per coding guidelines for grammar files, this properly reuses an existing rule rather than creating a new one.docs/user/ppl/cmd/lookup.md (1)
128-151: LGTM!Example 5 clearly demonstrates the OUTPUT keyword usage and explicitly states it's a synonym for REPLACE. The expected results correctly match Example 1, which reinforces the equivalence. Good documentation practice.
ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLLookupTest.java (2)
125-177: LGTM!The test comprehensively verifies OUTPUT behavior by checking:
- Logical plan generation matches REPLACE
- Result data is identical to
testReplace()- Spark SQL translation is correct
The comment on line 127 clearly documents the SPL compatibility purpose. Based on learnings, this properly tests SQL generation and optimization paths for Calcite integration.
179-231: LGTM!The
testOutputAs()test correctly verifies OUTPUT with field aliasing, mirroringtestReplaceAs()expectations. Good coverage for the aliasing scenario which is important for SPL compatibility.integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLLookupIT.java (2)
442-468: LGTM!Integration test properly verifies OUTPUT behavior in an end-to-end scenario using real OpenSearch indices. The expected results match
testUidAsIdReplaceDepartment(), confirming OUTPUT functions as a REPLACE synonym. As per coding guidelines, the test uses valid test data from resources loaded ininit().
470-495: LGTM!Good coverage for OUTPUT with field aliasing (
OUTPUT department AS country). The test complements the unit test by verifying the same behavior against actual OpenSearch indices. Expected results correctly match the REPLACE equivalent test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5049-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 425682f60d96750d34a9424ea7fb73b335a486ec
# Push it to GitHub
git push --set-upstream origin backport/backport-5049-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
|
@Swiddis could you help to complete the backporting please? I think it's needed for 2.19-dev |
3 participants
Description
Small syntax update.
Related Issues
N/A
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.