Skip to content

Commit c00b8b7

Browse files
anasalkouzanasalkouz
authored
Support PPL queries when having trailing pipes and/or empty pipes (#5161)
* Support PPL queries when having trailing pipes and/or empty pipes (#4032) Signed-off-by: Anas Alkouz <aalkouz@amazon.com> * 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 <aalkouz@amazon.com> * Addressing comments, adding negative test case Signed-off-by: Anas Alkouz <aalkouz@amazon.com> * Replace fragile toString with JSON comparison Signed-off-by: Anas Alkouz <aalkouz@amazon.com> --------- Signed-off-by: Anas Alkouz <aalkouz@amazon.com>
1 parent acf5fcb commit c00b8b7

4 files changed

Lines changed: 308 additions & 3 deletions

File tree

‎integ-test/src/test/java/org/opensearch/sql/ppl/TrailingPipeIT.java‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.ppl;
7+
8+
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
9+
10+
import java.io.IOException;
11+
import org.json.JSONObject;
12+
import org.junit.jupiter.api.Test;
13+
14+
/** Integration tests for trailing pipe and middle empty pipe functionality in PPL queries. */
15+
public class TrailingPipeIT extends PPLIntegTestCase {
16+
17+
/**
18+
* Initializes the test environment by loading the account index.
19+
*
20+
* @throws Exception if initialization fails
21+
*/
22+
@Override
23+
public void init() throws Exception {
24+
super.init();
25+
loadIndex(Index.ACCOUNT);
26+
}
27+
28+
/**
29+
* Tests that a trailing pipe after a source command produces identical results to a query without
30+
* the trailing pipe.
31+
*
32+
* @throws IOException if query execution fails
33+
*/
34+
@Test
35+
public void testTrailingPipeAfterSource() throws IOException {
36+
// Query with trailing pipe should produce same results as without
37+
JSONObject resultWithout = executeQuery(String.format("source=%s", TEST_INDEX_ACCOUNT));
38+
JSONObject resultWith = executeQuery(String.format("source=%s |", TEST_INDEX_ACCOUNT));
39+
40+
// Both should return the same data
41+
assertTrue(resultWithout.similar(resultWith));
42+
}
43+
44+
/**
45+
* Tests that a trailing pipe after a fields command produces identical results to a query without
46+
* the trailing pipe.
47+
*
48+
* @throws IOException if query execution fails
49+
*/
50+
@Test
51+
public void testTrailingPipeAfterFields() throws IOException {
52+
JSONObject resultWithout =
53+
executeQuery(
54+
String.format(
55+
"source=%s | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT));
56+
JSONObject resultWith =
57+
executeQuery(
58+
String.format(
59+
"source=%s | where age > 30 | fields firstname, age |", TEST_INDEX_ACCOUNT));
60+
61+
assertTrue(resultWithout.similar(resultWith));
62+
}
63+
64+
/**
65+
* Tests that a trailing pipe after a head command produces identical results to a query without
66+
* the trailing pipe.
67+
*
68+
* @throws IOException if query execution fails
69+
*/
70+
@Test
71+
public void testTrailingPipeAfterHead() throws IOException {
72+
JSONObject resultWithout =
73+
executeQuery(
74+
String.format("source=%s | fields firstname, age | head 3", TEST_INDEX_ACCOUNT));
75+
JSONObject resultWith =
76+
executeQuery(
77+
String.format("source=%s | fields firstname, age | head 3 |", TEST_INDEX_ACCOUNT));
78+
79+
assertTrue(resultWithout.similar(resultWith));
80+
}
81+
82+
/**
83+
* Tests that a trailing pipe after a complex query with multiple commands (where, fields, stats,
84+
* sort) produces identical results to a query without the trailing pipe.
85+
*
86+
* @throws IOException if query execution fails
87+
*/
88+
@Test
89+
public void testTrailingPipeWithComplexQuery() throws IOException {
90+
JSONObject resultWithout =
91+
executeQuery(
92+
String.format(
93+
"source=%s | where age > 25 | fields firstname, age, state | stats avg(age) by"
94+
+ " state | sort state",
95+
TEST_INDEX_ACCOUNT));
96+
JSONObject resultWith =
97+
executeQuery(
98+
String.format(
99+
"source=%s | where age > 25 | fields firstname, age, state | stats avg(age) by"
100+
+ " state | sort state |",
101+
TEST_INDEX_ACCOUNT));
102+
103+
assertTrue(resultWithout.similar(resultWith));
104+
}
105+
106+
/**
107+
* Tests that an empty pipe in the middle of a query pipeline is properly ignored and produces
108+
* identical results to a query without the empty pipe.
109+
*
110+
* @throws IOException if query execution fails
111+
*/
112+
@Test
113+
public void testEmptyPipeInMiddle() throws IOException {
114+
// Empty pipe in middle should be ignored
115+
JSONObject resultNormal =
116+
executeQuery(
117+
String.format(
118+
"source=%s | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT));
119+
JSONObject resultWithEmpty =
120+
executeQuery(
121+
String.format(
122+
"source=%s | | where age > 30 | fields firstname, age", TEST_INDEX_ACCOUNT));
123+
124+
assertTrue(resultNormal.similar(resultWithEmpty));
125+
}
126+
127+
/**
128+
* Tests that multiple empty pipes scattered throughout a query pipeline are properly ignored and
129+
* produce identical results to a query without the empty pipes.
130+
*
131+
* @throws IOException if query execution fails
132+
*/
133+
@Test
134+
public void testMultipleEmptyPipes() throws IOException {
135+
// Multiple empty pipes should be ignored
136+
JSONObject resultNormal =
137+
executeQuery(
138+
String.format(
139+
"source=%s | where age > 30 | fields firstname, age | sort age",
140+
TEST_INDEX_ACCOUNT));
141+
JSONObject resultWithEmpty =
142+
executeQuery(
143+
String.format(
144+
"source=%s | | where age > 30 | | fields firstname, age | sort age",
145+
TEST_INDEX_ACCOUNT));
146+
147+
assertTrue(resultNormal.similar(resultWithEmpty));
148+
}
149+
150+
/**
151+
* Tests that a combination of empty pipes in the middle and a trailing pipe at the end are
152+
* properly handled and produce identical results to a query without these extraneous pipes.
153+
*
154+
* @throws IOException if query execution fails
155+
*/
156+
@Test
157+
public void testEmptyPipesAndTrailingPipe() throws IOException {
158+
// Multiple empty pipes should be ignored
159+
JSONObject resultNormal =
160+
executeQuery(
161+
String.format(
162+
"source=%s | where age > 30 | fields firstname, age | sort age",
163+
TEST_INDEX_ACCOUNT));
164+
JSONObject resultWithEmpty =
165+
executeQuery(
166+
String.format(
167+
"source=%s | | where age > 30 | fields firstname, age | sort age |",
168+
TEST_INDEX_ACCOUNT));
169+
170+
assertTrue(resultNormal.similar(resultWithEmpty));
171+
}
172+
}

‎ppl/src/main/antlr/OpenSearchPPLParser.g4‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ pplStatement
2020
;
2121

2222
subPipeline
23-
: PIPE? commands (PIPE commands)*
23+
: PIPE? commands (PIPE commands?)*
2424
;
2525

2626
queryStatement
27-
: (PIPE)? pplCommands (PIPE commands)*
27+
: (PIPE)? pplCommands (PIPE commands?)*
2828
;
2929

3030
explainStatement
@@ -39,7 +39,7 @@ explainMode
3939
;
4040

4141
subSearch
42-
: searchCommand (PIPE commands)*
42+
: searchCommand (PIPE commands?)*
4343
;
4444

4545
// commands

‎ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -946,4 +946,55 @@ public void testAddTotalsCommandWithAllOptionsShouldPass() {
946946
.parse("source=t | addtotals price, quantity label='Total' labelfield='type'");
947947
assertNotEquals(null, tree);
948948
}
949+
950+
@Test
951+
public void testQueryWithMultipleTrailingPipesShouldPass() {
952+
// Multiple consecutive trailing pipes should be handled gracefully
953+
ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | fields a,b | |");
954+
assertNotEquals(null, tree);
955+
}
956+
957+
@Test
958+
public void testQueryWithTrailingPipeAndWhitespaceShouldPass() {
959+
ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | fields a,b | ");
960+
assertNotEquals(null, tree);
961+
}
962+
963+
@Test
964+
public void testQueryWithMiddleEmptyPipe() {
965+
ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | | fields a,b");
966+
assertNotEquals(null, tree);
967+
}
968+
969+
@Test
970+
public void testQueryWithMiddleEmptyPipeAndTrailingPipe() {
971+
ParseTree tree = new PPLSyntaxParser().parse("search source=t a=1 b=2 | | fields a,b | ");
972+
assertNotEquals(null, tree);
973+
}
974+
975+
@Test
976+
public void testComplexQueryWithTrailingPipeShouldPass() {
977+
ParseTree tree =
978+
new PPLSyntaxParser()
979+
.parse("source=t | where x > 5 | stats count() by status | sort -count |");
980+
assertNotEquals(null, tree);
981+
}
982+
983+
@Test
984+
public void testSubSearchWithTrailingPipeShouldPass() {
985+
ParseTree tree =
986+
new PPLSyntaxParser().parse("source=outer | join a [source=inner | fields x,y |]");
987+
assertNotEquals(null, tree);
988+
}
989+
990+
/**
991+
* Tests that the parser correctly rejects queries with invalid command tokens after a pipe,
992+
* ensuring proper error detection for malformed queries.
993+
*/
994+
@Test
995+
public void testPipeWithInvalidCommandShouldFail() {
996+
assertThrows(
997+
SyntaxCheckException.class,
998+
() -> new PPLSyntaxParser().parse("source=t | | 123invalidcommand"));
999+
}
9491000
}

‎ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1726,4 +1726,86 @@ public void testGraphLookupCommand() {
17261726
SemanticCheckException.class,
17271727
() -> plan("source=t | graphLookup employees fromField=manager as reportingHierarchy"));
17281728
}
1729+
1730+
@Test
1731+
public void testTrailingPipeAfterSource() {
1732+
// Test that trailing pipe after source produces same AST
1733+
assertEqual("source=t |", relation("t"));
1734+
}
1735+
1736+
@Test
1737+
public void testTrailingPipeAfterStats() {
1738+
// Test trailing pipe after stats command
1739+
assertEqual(
1740+
"source=t | stats count(a) by b |",
1741+
agg(
1742+
relation("t"),
1743+
exprList(alias("count(a)", aggregate("count", field("a")))),
1744+
emptyList(),
1745+
exprList(alias("b", field("b"))),
1746+
defaultStatsArgs()));
1747+
}
1748+
1749+
@Test
1750+
public void testTrailingPipeWithComplexQuery() {
1751+
// Test trailing pipe with complex query including where, stats, and sort
1752+
assertEqual(
1753+
"source=t | where a > 1 | stats count(b) by c | sort c |",
1754+
sort(
1755+
agg(
1756+
filter(relation("t"), compare(">", field("a"), intLiteral(1))),
1757+
exprList(alias("count(b)", aggregate("count", field("b")))),
1758+
emptyList(),
1759+
exprList(alias("c", field("c"))),
1760+
defaultStatsArgs()),
1761+
field("c", defaultSortFieldArgs())));
1762+
}
1763+
1764+
@Test
1765+
public void testEmptyPipeAfterSource() {
1766+
// Test that empty pipe after source is ignored
1767+
assertEqual("source=t | |", relation("t"));
1768+
}
1769+
1770+
@Test
1771+
public void testEmptyPipeInMiddle() {
1772+
// Test that empty pipe in middle is ignored
1773+
assertEqual(
1774+
"source=t | | where a=1", filter(relation("t"), compare("=", field("a"), intLiteral(1))));
1775+
}
1776+
1777+
@Test
1778+
public void testMultipleEmptyPipes() {
1779+
// Test multiple empty pipes are ignored
1780+
assertEqual(
1781+
"source=t | | where a=1 | | fields b | |",
1782+
projectWithArg(
1783+
filter(relation("t"), compare("=", field("a"), intLiteral(1))),
1784+
defaultFieldsArgs(),
1785+
field("b")));
1786+
}
1787+
1788+
/**
1789+
* Tests that a combination of empty pipes in the middle and a trailing pipe at the end are
1790+
* properly handled and produce the same AST as a query without these extraneous pipes.
1791+
*/
1792+
@Test
1793+
public void testEmptyPipeAndTrailingPipeTogether() {
1794+
// Test both empty pipe in middle and trailing pipe at end
1795+
assertEqual(
1796+
"source=t | | where a=1 | fields b |",
1797+
projectWithArg(
1798+
filter(relation("t"), compare("=", field("a"), intLiteral(1))),
1799+
defaultFieldsArgs(),
1800+
field("b")));
1801+
}
1802+
1803+
/**
1804+
* Tests that the parser correctly rejects queries with invalid command tokens after a pipe,
1805+
* ensuring proper error detection for malformed queries.
1806+
*/
1807+
@Test(expected = org.opensearch.sql.common.antlr.SyntaxCheckException.class)
1808+
public void testMalformedPipeProducesSyntaxError() {
1809+
plan("source=t | invalidCmd |");
1810+
}
17291811
}

0 commit comments

Comments
 (0)