From 1d821f53b825ec753ee1d898c996af574bf0c06f Mon Sep 17 00:00:00 2001 From: Anas Alkouz Date: Fri, 20 Feb 2026 10:05:06 -0800 Subject: [PATCH 1/4] Support PPL queries when having trailing pipes and/or empty pipes (#4032) Signed-off-by: Anas Alkouz --- .../opensearch/sql/ppl/TrailingPipeIT.java | 125 ++++++++++++++++++ ppl/src/main/antlr/OpenSearchPPLParser.g4 | 6 +- .../sql/ppl/antlr/PPLSyntaxParserTest.java | 41 ++++++ .../sql/ppl/parser/AstBuilderTest.java | 69 ++++++++++ 4 files changed, 238 insertions(+), 3 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java new file mode 100644 index 00000000000..1c34d10427f --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java @@ -0,0 +1,125 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ppl; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.jupiter.api.Test; + +/** Integration tests for trailing pipe and middle empty pipe functionality in PPL queries. */ +public class TrailingPipeIT extends PPLIntegTestCase { + + @Override + public void init() throws Exception { + super.init(); + loadIndex(Index.ACCOUNT); + } + + @Test + public void testTrailingPipeAfterSource() throws IOException { + // Query with trailing pipe should produce same results as without + JSONObject resultWithout = executeQuery(String.format("source=%s", TEST_INDEX_ACCOUNT)); + JSONObject resultWith = executeQuery(String.format("source=%s |", TEST_INDEX_ACCOUNT)); + + // Both should return the same data + assertEquals(resultWithout.toString(), resultWith.toString()); + } + + @Test + public void testTrailingPipeAfterFields() throws IOException { + JSONObject resultWithout = + executeQuery( + String.format( + "source=%s | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT)); + JSONObject resultWith = + executeQuery( + String.format( + "source=%s | where age > 30 | fields firstname, age |", TEST_INDEX_ACCOUNT)); + + assertEquals(resultWithout.toString(), resultWith.toString()); + } + + @Test + public void testTrailingPipeAfterHead() throws IOException { + JSONObject resultWithout = + executeQuery( + String.format("source=%s | fields firstname, age | head 3", TEST_INDEX_ACCOUNT)); + JSONObject resultWith = + executeQuery( + String.format("source=%s | fields firstname, age | head 3 |", TEST_INDEX_ACCOUNT)); + + assertEquals(resultWithout.toString(), resultWith.toString()); + } + + @Test + public void testTrailingPipeWithComplexQuery() throws IOException { + JSONObject resultWithout = + executeQuery( + String.format( + "source=%s | where age > 25 | fields firstname, age, state | stats avg(age) by" + + " state | sort state", + TEST_INDEX_ACCOUNT)); + JSONObject resultWith = + executeQuery( + String.format( + "source=%s | where age > 25 | fields firstname, age, state | stats avg(age) by" + + " state | sort state |", + TEST_INDEX_ACCOUNT)); + + assertEquals(resultWithout.toString(), resultWith.toString()); + } + + @Test + public void testEmptyPipeInMiddle() throws IOException { + // Empty pipe in middle should be ignored + JSONObject resultNormal = + executeQuery( + String.format( + "source=%s | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT)); + JSONObject resultWithEmpty = + executeQuery( + String.format( + "source=%s | | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT)); + + assertEquals(resultNormal.toString(), resultWithEmpty.toString()); + } + + @Test + public void testMultipleEmptyPipes() throws IOException { + // Multiple empty pipes should be ignored + JSONObject resultNormal = + executeQuery( + String.format( + "source=%s | where age > 30 | fields firstname, age | sort age", + TEST_INDEX_ACCOUNT)); + JSONObject resultWithEmpty = + executeQuery( + String.format( + "source=%s | | where age > 30 | | fields firstname, age | sort age", + TEST_INDEX_ACCOUNT)); + + assertEquals(resultNormal.toString(), resultWithEmpty.toString()); + } + + @Test + public void testEmptyPipesAndTrailingPipe() throws IOException { + // Multiple empty pipes should be ignored + JSONObject resultNormal = + executeQuery( + String.format( + "source=%s | where age > 30 | fields firstname, age | sort age", + TEST_INDEX_ACCOUNT)); + JSONObject resultWithEmpty = + executeQuery( + String.format( + "source=%s | | where age > 30 | fields firstname, age | sort age |", + TEST_INDEX_ACCOUNT)); + + assertEquals(resultNormal.toString(), resultWithEmpty.toString()); + } +} diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index 455fd92c3d2..bf3e90c72b2 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -20,11 +20,11 @@ pplStatement ; subPipeline - : PIPE? commands (PIPE commands)* + : PIPE? commands (PIPE commands?)* ; queryStatement - : (PIPE)? pplCommands (PIPE commands)* + : (PIPE)? pplCommands (PIPE commands?)* ; explainStatement @@ -39,7 +39,7 @@ explainMode ; subSearch - : searchCommand (PIPE commands)* + : searchCommand (PIPE commands?)* ; // commands diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java index dfe79c71edd..15607d0aa93 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java @@ -946,4 +946,45 @@ public void testAddTotalsCommandWithAllOptionsShouldPass() { .parse("source=t | addtotals price, quantity label='Total' labelfield='type'"); assertNotEquals(null, tree); } + + @Test + public void testQueryWithMultipleTrailingPipesShouldPass() { + // Multiple consecutive trailing pipes should be handled gracefully + // by allowing one optional trailing pipe + 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); + } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index 4c1980ee54e..1623be0cd12 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -1653,4 +1653,73 @@ public void testMvmapWithNonFieldFirstArgThrowsException() { () -> plan("source=t | eval result = mvmap(123, 123 * 10)")) .getMessage()); } + + @Test + public void testTrailingPipeAfterSource() { + // Test that trailing pipe after source produces same AST + assertEqual("source=t |", relation("t")); + } + + @Test + public void testTrailingPipeAfterStats() { + // Test trailing pipe after stats command + assertEqual( + "source=t | stats count(a) by b |", + agg( + relation("t"), + exprList(alias("count(a)", aggregate("count", field("a")))), + emptyList(), + exprList(alias("b", field("b"))), + defaultStatsArgs())); + } + + @Test + public void testTrailingPipeWithComplexQuery() { + // Test trailing pipe with complex query including where, stats, and sort + assertEqual( + "source=t | where a > 1 | stats count(b) by c | sort c |", + sort( + agg( + filter(relation("t"), compare(">", field("a"), intLiteral(1))), + exprList(alias("count(b)", aggregate("count", field("b")))), + emptyList(), + exprList(alias("c", field("c"))), + defaultStatsArgs()), + field("c", defaultSortFieldArgs()))); + } + + @Test + public void testEmptyPipeAfterSource() { + // Test that empty pipe after source is ignored + assertEqual("source=t | |", relation("t")); + } + + @Test + public void testEmptyPipeInMiddle() { + // Test that empty pipe in middle is ignored + assertEqual( + "source=t | | where a=1", filter(relation("t"), compare("=", field("a"), intLiteral(1)))); + } + + @Test + public void testMultipleEmptyPipes() { + // Test multiple empty pipes are ignored + assertEqual( + "source=t | | where a=1 | | fields b | |", + projectWithArg( + filter(relation("t"), compare("=", field("a"), intLiteral(1))), + defaultFieldsArgs(), + field("b"))); + } + + @Test + public void testEmptyPipeAndTrailingPipeTogether() { + // Test both empty pipe in middle and trailing pipe at end + assertEqual( + "source=t | | where a=1 | fields b |", + projectWithArg( + filter(relation("t"), compare("=", field("a"), intLiteral(1))), + defaultFieldsArgs(), + field("b"))); + } } From 6d1cbf6ef1bcec06a3e67bf5e6a4e42e42ccd775 Mon Sep 17 00:00:00 2001 From: Anas Alkouz Date: Tue, 24 Feb 2026 09:09:00 -0800 Subject: [PATCH 2/4] docs: Add comprehensive Javadoc comments to TrailingPipeIT test methods 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 #5161. Signed-off-by: Anas Alkouz --- .../opensearch/sql/ppl/TrailingPipeIT.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java index 1c34d10427f..0ec78d532e2 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java @@ -14,12 +14,23 @@ /** Integration tests for trailing pipe and middle empty pipe functionality in PPL queries. */ public class TrailingPipeIT extends PPLIntegTestCase { + /** + * Initializes the test environment by loading the account index. + * + * @throws Exception if initialization fails + */ @Override public void init() throws Exception { super.init(); loadIndex(Index.ACCOUNT); } + /** + * Tests that a trailing pipe after a source command produces identical results to a query without + * the trailing pipe. + * + * @throws IOException if query execution fails + */ @Test public void testTrailingPipeAfterSource() throws IOException { // Query with trailing pipe should produce same results as without @@ -30,6 +41,12 @@ public void testTrailingPipeAfterSource() throws IOException { assertEquals(resultWithout.toString(), resultWith.toString()); } + /** + * Tests that a trailing pipe after a fields command produces identical results to a query without + * the trailing pipe. + * + * @throws IOException if query execution fails + */ @Test public void testTrailingPipeAfterFields() throws IOException { JSONObject resultWithout = @@ -44,6 +61,12 @@ public void testTrailingPipeAfterFields() throws IOException { assertEquals(resultWithout.toString(), resultWith.toString()); } + /** + * Tests that a trailing pipe after a head command produces identical results to a query without + * the trailing pipe. + * + * @throws IOException if query execution fails + */ @Test public void testTrailingPipeAfterHead() throws IOException { JSONObject resultWithout = @@ -56,6 +79,12 @@ public void testTrailingPipeAfterHead() throws IOException { assertEquals(resultWithout.toString(), resultWith.toString()); } + /** + * Tests that a trailing pipe after a complex query with multiple commands (where, fields, stats, + * sort) produces identical results to a query without the trailing pipe. + * + * @throws IOException if query execution fails + */ @Test public void testTrailingPipeWithComplexQuery() throws IOException { JSONObject resultWithout = @@ -74,6 +103,12 @@ public void testTrailingPipeWithComplexQuery() throws IOException { assertEquals(resultWithout.toString(), resultWith.toString()); } + /** + * Tests that an empty pipe in the middle of a query pipeline is properly ignored and produces + * identical results to a query without the empty pipe. + * + * @throws IOException if query execution fails + */ @Test public void testEmptyPipeInMiddle() throws IOException { // Empty pipe in middle should be ignored @@ -89,6 +124,12 @@ public void testEmptyPipeInMiddle() throws IOException { assertEquals(resultNormal.toString(), resultWithEmpty.toString()); } + /** + * Tests that multiple empty pipes scattered throughout a query pipeline are properly ignored and + * produce identical results to a query without the empty pipes. + * + * @throws IOException if query execution fails + */ @Test public void testMultipleEmptyPipes() throws IOException { // Multiple empty pipes should be ignored @@ -106,6 +147,12 @@ public void testMultipleEmptyPipes() throws IOException { assertEquals(resultNormal.toString(), resultWithEmpty.toString()); } + /** + * Tests that a combination of empty pipes in the middle and a trailing pipe at the end are + * properly handled and produce identical results to a query without these extraneous pipes. + * + * @throws IOException if query execution fails + */ @Test public void testEmptyPipesAndTrailingPipe() throws IOException { // Multiple empty pipes should be ignored From 7c0a3733d209346c038e77cb5451e00152d410ad Mon Sep 17 00:00:00 2001 From: Anas Alkouz Date: Tue, 24 Feb 2026 12:45:12 -0800 Subject: [PATCH 3/4] Addressing comments, adding negative test case Signed-off-by: Anas Alkouz --- .../sql/ppl/antlr/PPLSyntaxParserTest.java | 12 +++++++++++- .../opensearch/sql/ppl/parser/AstBuilderTest.java | 13 +++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java index 15607d0aa93..1238f1e7e6c 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java @@ -950,7 +950,6 @@ public void testAddTotalsCommandWithAllOptionsShouldPass() { @Test public void testQueryWithMultipleTrailingPipesShouldPass() { // Multiple consecutive trailing pipes should be handled gracefully - // by allowing one optional trailing pipe ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | fields a,b | |"); assertNotEquals(null, tree); } @@ -987,4 +986,15 @@ public void testSubSearchWithTrailingPipeShouldPass() { new PPLSyntaxParser().parse("source=outer | join a [source=inner | fields x,y |]"); assertNotEquals(null, tree); } + + /** + * Tests that the parser correctly rejects queries with invalid command tokens after a pipe, + * ensuring proper error detection for malformed queries. + */ + @Test + public void testPipeWithInvalidCommandShouldFail() { + assertThrows( + SyntaxCheckException.class, + () -> new PPLSyntaxParser().parse("source=t | | 123invalidcommand")); + } } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index 1623be0cd12..cc82d77c46c 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -1712,6 +1712,10 @@ public void testMultipleEmptyPipes() { field("b"))); } + /** + * Tests that a combination of empty pipes in the middle and a trailing pipe at the end are + * properly handled and produce the same AST as a query without these extraneous pipes. + */ @Test public void testEmptyPipeAndTrailingPipeTogether() { // Test both empty pipe in middle and trailing pipe at end @@ -1722,4 +1726,13 @@ public void testEmptyPipeAndTrailingPipeTogether() { defaultFieldsArgs(), field("b"))); } + + /** + * Tests that the parser correctly rejects queries with invalid command tokens after a pipe, + * ensuring proper error detection for malformed queries. + */ + @Test(expected = org.opensearch.sql.common.antlr.SyntaxCheckException.class) + public void testMalformedPipeProducesSyntaxError() { + plan("source=t | invalidCmd |"); + } } From e74ba19dac729e3015dcf2a139d76017cd09a9b3 Mon Sep 17 00:00:00 2001 From: Anas Alkouz Date: Tue, 24 Feb 2026 13:12:32 -0800 Subject: [PATCH 4/4] Replace fragile toString with JSON comparison Signed-off-by: Anas Alkouz --- .../org/opensearch/sql/ppl/TrailingPipeIT.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java index 0ec78d532e2..8b5df6e710e 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java @@ -38,7 +38,7 @@ public void testTrailingPipeAfterSource() throws IOException { JSONObject resultWith = executeQuery(String.format("source=%s |", TEST_INDEX_ACCOUNT)); // Both should return the same data - assertEquals(resultWithout.toString(), resultWith.toString()); + assertTrue(resultWithout.similar(resultWith)); } /** @@ -58,7 +58,7 @@ public void testTrailingPipeAfterFields() throws IOException { String.format( "source=%s | where age > 30 | fields firstname, age |", TEST_INDEX_ACCOUNT)); - assertEquals(resultWithout.toString(), resultWith.toString()); + assertTrue(resultWithout.similar(resultWith)); } /** @@ -76,7 +76,7 @@ public void testTrailingPipeAfterHead() throws IOException { executeQuery( String.format("source=%s | fields firstname, age | head 3 |", TEST_INDEX_ACCOUNT)); - assertEquals(resultWithout.toString(), resultWith.toString()); + assertTrue(resultWithout.similar(resultWith)); } /** @@ -100,7 +100,7 @@ public void testTrailingPipeWithComplexQuery() throws IOException { + " state | sort state |", TEST_INDEX_ACCOUNT)); - assertEquals(resultWithout.toString(), resultWith.toString()); + assertTrue(resultWithout.similar(resultWith)); } /** @@ -121,7 +121,7 @@ public void testEmptyPipeInMiddle() throws IOException { String.format( "source=%s | | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT)); - assertEquals(resultNormal.toString(), resultWithEmpty.toString()); + assertTrue(resultNormal.similar(resultWithEmpty)); } /** @@ -144,7 +144,7 @@ public void testMultipleEmptyPipes() throws IOException { "source=%s | | where age > 30 | | fields firstname, age | sort age", TEST_INDEX_ACCOUNT)); - assertEquals(resultNormal.toString(), resultWithEmpty.toString()); + assertTrue(resultNormal.similar(resultWithEmpty)); } /** @@ -167,6 +167,6 @@ public void testEmptyPipesAndTrailingPipe() throws IOException { "source=%s | | where age > 30 | fields firstname, age | sort age |", TEST_INDEX_ACCOUNT)); - assertEquals(resultNormal.toString(), resultWithEmpty.toString()); + assertTrue(resultNormal.similar(resultWithEmpty)); } }