Support PPL queries when having trailing pipes and/or empty pipes#5161
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughMakes pipe-following commands optional in the PPL grammar and adds unit and integration tests to ensure trailing and empty pipes are accepted and ignored during parsing, AST building, and execution. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Parser as Parser\n(OpenSearchPPLParser)
participant AST as ASTBuilder
participant Exec as Executor
participant Index as DataStore
Client->>Parser: submit PPL query (may end with pipe)
Parser-->>Client: produce parse tree (allows empty command after PIPE)
Parser->>AST: build AST (ignore empty pipeline nodes)
AST-->>Parser: AST
AST->>Exec: execute AST
Exec->>Index: query index (e.g., ACCOUNT)
Index-->>Exec: results
Exec-->>Client: final query results (same as without trailing pipe)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
anasalkouz
added
enhancement
anasalkouz
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java (1)
950-989: Missing negative test case for the new grammar behaviour.All new tests verify that parse trees are produced (positive cases). Per coding guidelines, grammar tests should include negative cases. Consider adding a test verifying that an invalid command token following a pipe still raises a
SyntaxCheckException, so regressions in error detection are caught:`@Test`(expected = SyntaxCheckException.class) public void testPipeWithInvalidCommandShouldFail() { new PPLSyntaxParser().parse("source=t | | 123invalidcommand"); }Based on learnings: "Applies to
**/{grammar,parser}/**/*Test.java: Include edge cases and boundary conditions in parser and grammar tests" and "Applies to**/{grammar,parser}/**/*.java: Test new grammar rules with positive and negative cases for parser development".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java` around lines 950 - 989, Add a negative parser test to ensure invalid tokens after a pipe throw SyntaxCheckException: create a new test method (e.g., testPipeWithInvalidCommandShouldFail) that calls new PPLSyntaxParser().parse("source=t | | 123invalidcommand") and expects SyntaxCheckException; place it alongside the existing positive tests (those invoking PPLSyntaxParser.parse in PPLSyntaxParserTest) so grammar regressions are caught.ppl/src/main/antlr/OpenSearchPPLParser.g4 (1)
568-574:appendcolCommandandappendCommandnot updated — inconsistent trailing-pipe behaviour.After this change,
appendpipe [ stats count() | ]succeeds (viasubPipeline), but the equivalentappendcol [ stats count() | ]still fails becauseappendcolCommandandappendCommandstill use the old(PIPE commands)*pattern (command required after pipe).🔧 Suggested fix to align `appendcolCommand` with the PR's intent
appendcolCommand - : APPENDCOL (OVERRIDE EQUAL override = booleanLiteral)? LT_SQR_PRTHS commands (PIPE commands)* RT_SQR_PRTHS + : APPENDCOL (OVERRIDE EQUAL override = booleanLiteral)? LT_SQR_PRTHS commands (PIPE commands?)* RT_SQR_PRTHS ; appendCommand - : APPEND LT_SQR_PRTHS searchCommand? (PIPE commands)* RT_SQR_PRTHS + : APPEND LT_SQR_PRTHS searchCommand? (PIPE commands?)* RT_SQR_PRTHS ;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/main/antlr/OpenSearchPPLParser.g4` around lines 568 - 574, The grammar rules appendcolCommand and appendCommand still require a commands after every PIPE (using (PIPE commands)*), causing appendcol [...] with a trailing pipe to fail; update both rules to allow a PIPE with optional commands by changing the repetition from (PIPE commands)* to (PIPE commands?)* (i.e., make commands optional after PIPE) so trailing pipes are accepted; reference rules: appendcolCommand, appendCommand, token PIPE and rule commands.ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java (1)
1657-1724: Add at least one negative test for completeness.The new tests cover positive cases well. Per guidelines, parser/grammar tests should include both positive and negative cases. Consider adding a test verifying that a genuinely malformed pipe sequence (e.g., pipe with an invalid command token still present) produces a
SyntaxCheckException.Additionally, there is no coverage for the
subPipelinecontext (i.e., trailing pipe insideappendpipe [...]). SinceappendPipeCommandusessubPipeline, which was also updated, a test like:`@Test` public void testTrailingPipeInAppendPipe() { assertEqual( "source=t | appendpipe [ stats count() | ]", /* expected AST for appendpipe */ ... ); }would close the coverage gap.
Based on learnings: "Applies to
**/{grammar,parser}/**/*Test.java: Include edge cases and boundary conditions in parser and grammar tests" and "Applies to**/{grammar,parser}/**/*.java: Test new grammar rules with positive and negative cases for parser development".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java` around lines 1657 - 1724, Add negative and subPipeline coverage in AstBuilderTest: add a test method (e.g., testMalformedPipeProducesSyntaxError) that parses a deliberately malformed pipeline like "source=t | invalidCmd |" and asserts a SyntaxCheckException is thrown (use the test harness/assertThrows pattern used elsewhere and reference SyntaxCheckException). Also add a positive test (e.g., testTrailingPipeInAppendPipe) for the appendpipe/subPipeline case using the existing AST helper builders (relation(...), agg(...), projectWithArg(...), etc.) to assert that "source=t | appendpipe [ stats count() | ]" produces the expected AST (reference the grammar rule appendPipeCommand and subPipeline to locate the parser behavior to exercise). Ensure both tests are added to AstBuilderTest with clear names so future diffs cover negative and subPipeline boundary cases.integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java (1)
23-31:toString()JSON comparison is fragile — prefer a dedicated equality helper.
JSONObject.toString()ordering is not guaranteed to be stable across library versions or platforms. If the response JSON keys reorder between calls, the assertion would produce a false failure even when the data is identical.Other integration tests in this codebase use structured comparison helpers. Consider comparing the
totalcount and a sample of hits, or use a deep-equality helper available in the base class, rather than relying on string serialization order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java` around lines 23 - 31, The testTrailingPipeAfterSource relies on JSONObject.toString() ordering; instead assert structural equality: call executeQuery(...) into resultWithout/resultWith and compare resultWithout.getInt("total") with resultWith.getInt("total") and compare the hits arrays element-wise (e.g., getJSONArray("hits") and compare lengths and each JSONObject entry using JSONObject.equals or a deep-equality helper from the test base). Replace the assertEquals(resultWithout.toString(), resultWith.toString()) with these targeted assertions so ordering of serialized keys won't cause false failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java`:
- Around line 950-956: Update the misleading test comment in
testQueryWithMultipleTrailingPipesShouldPass: the test string passed to
PPLSyntaxParser.parse ("search source=t a=1 b=2 | fields a,b | |") contains two
consecutive empty pipe tokens (an empty pipeline segment plus a trailing empty
pipe), so change the comment to accurately state that the test verifies handling
of two consecutive trailing pipes (an empty middle pipe and a trailing empty
pipe) rather than "one optional trailing pipe"; keep the assertion and parse
call unchanged.
---
Nitpick comments:
In `@integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java`:
- Around line 23-31: The testTrailingPipeAfterSource relies on
JSONObject.toString() ordering; instead assert structural equality: call
executeQuery(...) into resultWithout/resultWith and compare
resultWithout.getInt("total") with resultWith.getInt("total") and compare the
hits arrays element-wise (e.g., getJSONArray("hits") and compare lengths and
each JSONObject entry using JSONObject.equals or a deep-equality helper from the
test base). Replace the assertEquals(resultWithout.toString(),
resultWith.toString()) with these targeted assertions so ordering of serialized
keys won't cause false failures.
In `@ppl/src/main/antlr/OpenSearchPPLParser.g4`:
- Around line 568-574: The grammar rules appendcolCommand and appendCommand
still require a commands after every PIPE (using (PIPE commands)*), causing
appendcol [...] with a trailing pipe to fail; update both rules to allow a PIPE
with optional commands by changing the repetition from (PIPE commands)* to (PIPE
commands?)* (i.e., make commands optional after PIPE) so trailing pipes are
accepted; reference rules: appendcolCommand, appendCommand, token PIPE and rule
commands.
In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java`:
- Around line 950-989: Add a negative parser test to ensure invalid tokens after
a pipe throw SyntaxCheckException: create a new test method (e.g.,
testPipeWithInvalidCommandShouldFail) that calls new
PPLSyntaxParser().parse("source=t | | 123invalidcommand") and expects
SyntaxCheckException; place it alongside the existing positive tests (those
invoking PPLSyntaxParser.parse in PPLSyntaxParserTest) so grammar regressions
are caught.
In `@ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java`:
- Around line 1657-1724: Add negative and subPipeline coverage in
AstBuilderTest: add a test method (e.g., testMalformedPipeProducesSyntaxError)
that parses a deliberately malformed pipeline like "source=t | invalidCmd |" and
asserts a SyntaxCheckException is thrown (use the test harness/assertThrows
pattern used elsewhere and reference SyntaxCheckException). Also add a positive
test (e.g., testTrailingPipeInAppendPipe) for the appendpipe/subPipeline case
using the existing AST helper builders (relation(...), agg(...),
projectWithArg(...), etc.) to assert that "source=t | appendpipe [ stats count()
| ]" produces the expected AST (reference the grammar rule appendPipeCommand and
subPipeline to locate the parser behavior to exercise). Ensure both tests are
added to AstBuilderTest with clear names so future diffs cover negative and
subPipeline boundary cases.
Add detailed Javadoc documentation to all test methods in TrailingPipeIT class to improve code documentation coverage and meet the 80% threshold requirement. Each test method now includes: - Clear description of what the test validates - @throws IOException annotation for exception handling This addresses the docstring coverage issue flagged by CodeRabbit in PR opensearch-project#5161.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java (1)
950-955: Comment still inaccurately describes the test query.Line 952 reads "Multiple consecutive trailing pipes should be handled gracefully," but the query
"search source=t a=1 b=2 | fields a,b | |"has a normal| fields a,bstage, then an empty middle pipe, then a trailing pipe — the first|is not trailing at all.✏️ Suggested comment fix
- // Multiple consecutive trailing pipes should be handled gracefully + // Empty middle pipe followed by a trailing pipe should both parse successfully🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java` around lines 950 - 955, The test comment for testQueryWithMultipleTrailingPipesShouldPass is inaccurate for the query "search source=t a=1 b=2 | fields a,b | |" because the first '|' is not trailing (there is a normal "fields" stage, an empty middle stage, then a trailing pipe); update either the test name (e.g., testQueryWithEmptyStageAndTrailingPipeShouldPass) or the inline comment to accurately state "Query contains an empty intermediate stage and a trailing pipe" so the description matches the actual query in the test method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java`:
- Around line 950-988: Add missing boundary-case unit tests in
PPLSyntaxParserTest: create a test method (e.g.,
testSingleSourceTrailingPipeShouldPass) that calls new
PPLSyntaxParser().parse("source=t |") and asserts the ParseTree is not null, and
another test (e.g., testThreeConsecutiveEmptyPipesShouldPass) that parses a
query with three or more consecutive trailing pipes (e.g., "source=t | | |") and
asserts non-null. Place these alongside the existing tests so they exercise
PPLSyntaxParser.parse and cover the canonical single-source trailing pipe and
the repetition boundary for (PIPE commands?)*.
---
Duplicate comments:
In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java`:
- Around line 950-955: The test comment for
testQueryWithMultipleTrailingPipesShouldPass is inaccurate for the query "search
source=t a=1 b=2 | fields a,b | |" because the first '|' is not trailing (there
is a normal "fields" stage, an empty middle stage, then a trailing pipe); update
either the test name (e.g., testQueryWithEmptyStageAndTrailingPipeShouldPass) or
the inline comment to accurately state "Query contains an empty intermediate
stage and a trailing pipe" so the description matches the actual query in the
test method.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.javappl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.javappl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java
- integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java
| @Test | ||
| public void testQueryWithMultipleTrailingPipesShouldPass() { | ||
| // Multiple consecutive trailing pipes should be handled gracefully | ||
| ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | fields a,b | |"); | ||
| assertNotEquals(null, tree); | ||
| } | ||
|
|
||
| @Test | ||
| public void testQueryWithTrailingPipeAndWhitespaceShouldPass() { | ||
| ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | fields a,b | "); | ||
| assertNotEquals(null, tree); | ||
| } | ||
|
|
||
| @Test | ||
| public void testQueryWithMiddleEmptyPipe() { | ||
| ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | | fields a,b"); | ||
| assertNotEquals(null, tree); | ||
| } | ||
|
|
||
| @Test | ||
| public void testQueryWithMiddleEmptyPipeAndTrailingPipe() { | ||
| ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | | fields a,b | "); | ||
| assertNotEquals(null, tree); | ||
| } | ||
|
|
||
| @Test | ||
| public void testComplexQueryWithTrailingPipeShouldPass() { | ||
| ParseTree tree = | ||
| new PPLSyntaxParser() | ||
| .parse("source=t | where x > 5 | stats count() by status | sort -count |"); | ||
| assertNotEquals(null, tree); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSubSearchWithTrailingPipeShouldPass() { | ||
| ParseTree tree = | ||
| new PPLSyntaxParser().parse("source=outer | join a [source=inner | fields x,y |]"); | ||
| assertNotEquals(null, tree); | ||
| } |
There was a problem hiding this comment.
Missing minimal "source=t |" test — the canonical scenario from issue #4032.
None of the new positive tests cover the simplest single-source trailing pipe (source=t |), which is the exact motivating example from the linked issue. testComplexQueryWithTrailingPipeShouldPass does end with |, but burying the baseline scenario inside a four-stage pipeline makes it a less obvious regression anchor. Adding a standalone test for this, and for three or more consecutive empty pipes (boundary of the (PIPE commands?)* repetition), would satisfy the boundary-condition requirement.
✏️ Suggested additions
+ `@Test`
+ public void testQueryWithSingleTrailingPipeShouldPass() {
+ // Minimal case: single trailing pipe (motivating example from issue `#4032`)
+ ParseTree tree = new PPLSyntaxParser().parse("source=t |");
+ assertNotEquals(null, tree);
+ }
+
+ `@Test`
+ public void testQueryWithMultipleConsecutiveEmptyPipesShouldPass() {
+ // Boundary: three or more consecutive empty/trailing pipes
+ ParseTree tree = new PPLSyntaxParser().parse("source=t | | | fields a");
+ assertNotEquals(null, tree);
+ }Based on learnings: "Include edge cases and boundary conditions in parser and grammar tests" and "Test new grammar rules with positive and negative cases for parser development."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java`
around lines 950 - 988, Add missing boundary-case unit tests in
PPLSyntaxParserTest: create a test method (e.g.,
testSingleSourceTrailingPipeShouldPass) that calls new
PPLSyntaxParser().parse("source=t |") and asserts the ParseTree is not null, and
another test (e.g., testThreeConsecutiveEmptyPipesShouldPass) that parses a
query with three or more consecutive trailing pipes (e.g., "source=t | | |") and
asserts non-null. Place these alongside the existing tests so they exercise
PPLSyntaxParser.parse and cover the canonical single-source trailing pipe and
the repetition boundary for (PIPE commands?)*.
|
Please check and fix the DCO failure. Thanks! |
…ensearch-project#4032) Signed-off-by: Anas Alkouz <aalkouz@amazon.com>
Add detailed Javadoc documentation to all test methods in TrailingPipeIT class to improve code documentation coverage and meet the 80% threshold requirement. Each test method now includes: - Clear description of what the test validates - @throws IOException annotation for exception handling This addresses the docstring coverage issue flagged by CodeRabbit in PR opensearch-project#5161. Signed-off-by: Anas Alkouz <aalkouz@amazon.com>
Signed-off-by: Anas Alkouz <aalkouz@amazon.com>
Signed-off-by: Anas Alkouz <aalkouz@amazon.com>
anasalkouz
force-pushed
the
feature/ignoreTrailingPipe
branch
from
0e71348 to
e74ba19
Compare
PR Reviewer Guide 🔍(Review updated until commit 5bea79f)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 5bea79f
Previous suggestionsSuggestions up to commit e74ba19
|
anasalkouz
force-pushed
the
feature/ignoreTrailingPipe
branch
from
e74ba19 to
acf5fcb
Compare
Signed-off-by: Anas Alkouz <aalkouz@amazon.com>
|
Persistent review updated to latest commit 5bea79f |
1 similar comment
|
Persistent review updated to latest commit 5bea79f |
4 participants
Signed-off-by: Anas Alkouz aalkouz@amazon.com
Description
This PR enhances the PPL (Piped Processing Language) grammar to support trailing pipes at the end of queries and empty pipes in the middle of command sequences. This improvement makes the PPL syntax more forgiving and user-friendly.
Related Issues
Resolves #4032
Motivation
Users often accidentally add trailing pipes when writing queries, especially when:
Previously, these queries would fail with syntax errors. This change allows such queries to execute successfully, improving the overall user experience.
Changes
Grammar Modifications
File:
ppl/src/main/antlr/OpenSearchPPLParser.g4Modified two key grammar rules to support optional trailing and empty pipes:
The pattern
(PIPE commands?)*elegantly handles:Test Coverage
1. Syntax Parser Tests
File:
ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.javaAdded tests for:
2. AST Builder Tests
File:
ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.javaAdded tests to verify correct AST generation for:
3. Integration Tests
File:
integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.javaCreated integration tests that validate:
Examples
Before (Would Fail)
After (Now Works)
Empty Pipes (Also Supported)
Testing
./gradlew spotlessApplyCheck 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.