Skip to content

Commit d2b35ee

Browse files
ahkcsclaude
andcommitted
[Bug Fix] Explicit head command should override fetch_size in PPL
When a user's PPL query contains an explicit head command, the fetch_size parameter should not inject an additional Head node on top. Previously, fetch_size always wrapped the plan with Head(fetchSize), creating a double-Head that could cap results unexpectedly (e.g., head 100 with fetch_size=5 would only return 5 rows). This adds a containsHead() check in AstStatementBuilder to skip the Head injection when the user's query already has a head command, letting the user's explicit intent take precedence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kai Huang <ahkcs@amazon.com>
1 parent acf5fcb commit d2b35ee

3 files changed

Lines changed: 73 additions & 15 deletions

File tree

integ-test/src/test/java/org/opensearch/sql/ppl/FetchSizeIT.java

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,9 @@ public void testFetchSizeWithStats() throws IOException {
172172
}
173173

174174
@Test
175-
public void testFetchSizeWithHead() throws IOException {
176-
// Both head command and fetch_size - the smaller limit should win
177-
// head 3 limits to 3, fetch_size 10 would allow 10, so we get 3
175+
public void testHeadOverridesFetchSizeWhenSmaller() throws IOException {
176+
// Explicit head takes precedence over fetch_size
177+
// head 3 returns 3 rows regardless of fetch_size=10
178178
JSONObject result =
179179
executeQueryWithFetchSize(
180180
String.format("source=%s | head 3 | fields firstname", TEST_INDEX_ACCOUNT), 10);
@@ -183,16 +183,59 @@ public void testFetchSizeWithHead() throws IOException {
183183
}
184184

185185
@Test
186-
public void testFetchSizeSmallerThanHead() throws IOException {
187-
// fetch_size smaller than head - fetch_size should further limit
188-
// head 100 would return 100, but fetch_size 5 limits to 5
186+
public void testHeadOverridesFetchSizeWhenLarger() throws IOException {
187+
// Explicit head takes precedence over fetch_size
188+
// head 100 should return 100 rows even though fetch_size=5
189189
JSONObject result =
190190
executeQueryWithFetchSize(
191191
String.format("source=%s | head 100 | fields firstname", TEST_INDEX_ACCOUNT), 5);
192192
JSONArray dataRows = result.getJSONArray("datarows");
193+
assertEquals(100, dataRows.length());
194+
}
195+
196+
@Test
197+
public void testHeadOverridesFetchSizeWithOffset() throws IOException {
198+
// Explicit head with offset takes precedence over fetch_size
199+
// head 3 from 2 should skip 2 rows and return 3 rows, ignoring fetch_size=100
200+
JSONObject result =
201+
executeQueryWithFetchSize(
202+
String.format("source=%s | head 3 from 2 | fields firstname", TEST_INDEX_ACCOUNT), 100);
203+
JSONArray dataRows = result.getJSONArray("datarows");
204+
assertEquals(3, dataRows.length());
205+
}
206+
207+
@Test
208+
public void testHeadOverridesFetchSizeWithFilter() throws IOException {
209+
// Explicit head after filter takes precedence over fetch_size
210+
// Even with fetch_size=2, head 5 should return 5 matching rows
211+
JSONObject result =
212+
executeQueryWithFetchSize(
213+
String.format(
214+
"source=%s | where age > 30 | head 5 | fields firstname, age", TEST_INDEX_ACCOUNT),
215+
2);
216+
JSONArray dataRows = result.getJSONArray("datarows");
193217
assertEquals(5, dataRows.length());
194218
}
195219

220+
@Test
221+
public void testHeadEqualToFetchSize() throws IOException {
222+
// When head and fetch_size are the same value, head takes precedence (no double-Head)
223+
JSONObject result =
224+
executeQueryWithFetchSize(
225+
String.format("source=%s | head 7 | fields firstname", TEST_INDEX_ACCOUNT), 7);
226+
JSONArray dataRows = result.getJSONArray("datarows");
227+
assertEquals(7, dataRows.length());
228+
}
229+
230+
@Test
231+
public void testHeadLargerThanDatasetWithFetchSize() throws IOException {
232+
// head 1000 on a 7-row index with fetch_size=3: head takes precedence, returns all 7 rows
233+
JSONObject result =
234+
executeQueryWithFetchSize(String.format("source=%s | head 1000", TEST_INDEX_BANK), 3);
235+
JSONArray dataRows = result.getJSONArray("datarows");
236+
assertEquals(7, dataRows.length());
237+
}
238+
196239
@Test
197240
public void testFetchSizeAsUrlParameter() throws IOException {
198241
// fetch_size specified as URL parameter instead of JSON body

ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public class AstStatementBuilder extends OpenSearchPPLParserBaseVisitor<Statemen
3232
@Override
3333
public Statement visitPplStatement(OpenSearchPPLParser.PplStatementContext ctx) {
3434
UnresolvedPlan rawPlan = astBuilder.visit(ctx);
35-
if (context.getFetchSize() > 0) {
35+
if (context.getFetchSize() > 0 && !containsHead(rawPlan)) {
3636
rawPlan = new Head(context.getFetchSize(), 0).attach(rawPlan);
3737
}
3838
UnresolvedPlan plan = addSelectAll(rawPlan);
@@ -69,6 +69,23 @@ public static class StatementBuilderContext {
6969
private final String explainMode;
7070
}
7171

72+
/**
73+
* Recursively checks if the AST contains a {@link Head} node. When the user's query already
74+
* includes an explicit {@code head} command, we should not inject an additional Head for
75+
* fetch_size so that the user's explicit limit takes precedence.
76+
*/
77+
private boolean containsHead(UnresolvedPlan plan) {
78+
if (plan instanceof Head) {
79+
return true;
80+
}
81+
for (var child : plan.getChild()) {
82+
if (child instanceof UnresolvedPlan && containsHead((UnresolvedPlan) child)) {
83+
return true;
84+
}
85+
}
86+
return false;
87+
}
88+
7289
private UnresolvedPlan addSelectAll(UnresolvedPlan plan) {
7390
if ((plan instanceof Project) && !((Project) plan).isExcluded()) {
7491
return plan;

ppl/src/test/java/org/opensearch/sql/ppl/parser/AstStatementBuilderTest.java

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,33 +87,31 @@ public void buildQueryStatementWithLargeFetchSize() {
8787
@Test
8888
public void buildQueryStatementWithFetchSizeAndSmallerHead() {
8989
// User query has head 3, fetchSize=10
90-
// Head(10) wraps Head(3), then Project(*) wraps on top
91-
// The inner head 3 limits first, so only 3 rows are returned
90+
// Explicit head takes precedence over fetch_size, so no outer Head(10) is injected
9291
assertEqualWithFetchSize(
9392
"source=t | head 3",
9493
10,
95-
new Query(project(head(head(relation("t"), 3, 0), 10, 0), AllFields.of()), 0, PPL));
94+
new Query(project(head(relation("t"), 3, 0), AllFields.of()), 0, PPL));
9695
}
9796

9897
@Test
9998
public void buildQueryStatementWithFetchSizeSmallerThanHead() {
10099
// User query has head 100, fetchSize=5
101-
// Head(5) wraps Head(100), then Project(*) wraps on top
102-
// The outer head 5 limits, so only 5 rows are returned
100+
// Explicit head takes precedence over fetch_size, so no outer Head(5) is injected
103101
assertEqualWithFetchSize(
104102
"source=t | head 100",
105103
5,
106-
new Query(project(head(head(relation("t"), 100, 0), 5, 0), AllFields.of()), 0, PPL));
104+
new Query(project(head(relation("t"), 100, 0), AllFields.of()), 0, PPL));
107105
}
108106

109107
@Test
110108
public void buildQueryStatementWithFetchSizeAndHeadWithOffset() {
111109
// User query has head 3 from 1 (with offset), fetchSize=10
112-
// The inner head offset is preserved, outer Head always has offset 0
110+
// Explicit head takes precedence over fetch_size, so no outer Head(10) is injected
113111
assertEqualWithFetchSize(
114112
"source=t | head 3 from 1",
115113
10,
116-
new Query(project(head(head(relation("t"), 3, 1), 10, 0), AllFields.of()), 0, PPL));
114+
new Query(project(head(relation("t"), 3, 1), AllFields.of()), 0, PPL));
117115
}
118116

119117
private void assertEqualWithFetchSize(String query, int fetchSize, Statement expectedStatement) {

0 commit comments

Comments
 (0)