Skip to content

Commit 29344e5

Browse files
Yury-Fridlyandgithub-actions[bot]
Yury-Fridlyand
authored and
github-actions[bot]
committed
Pagination Phase 2: Support ORDER BY clauses and queries without FROM. (#1599)
* Support `ORDER BY` clauses in pagination and queries without `FROM`. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Fix IT. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> --------- Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> (cherry picked from commit 94d5479)
1 parent d2a82ae commit 29344e5

8 files changed

Lines changed: 217 additions & 52 deletions

File tree

‎core/src/main/java/org/opensearch/sql/executor/pagination/CanPaginateVisitor.java‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@
4848
* Currently, V2 engine does not support queries with:
4949
* - aggregation (GROUP BY clause or aggregation functions like min/max)
5050
* - in memory aggregation (window function)
51-
* - ORDER BY clause
5251
* - LIMIT/OFFSET clause(s)
5352
* - without FROM clause
5453
* - JOIN
@@ -68,14 +67,24 @@ public Boolean visitRelation(Relation node, Object context) {
6867
return Boolean.TRUE;
6968
}
7069

71-
private Boolean canPaginate(Node node, Object context) {
70+
protected Boolean canPaginate(Node node, Object context) {
7271
var childList = node.getChild();
7372
if (childList != null) {
7473
return childList.stream().allMatch(n -> n.accept(this, context));
7574
}
7675
return Boolean.TRUE;
7776
}
7877

78+
// Only column references in ORDER BY clause are supported in pagination,
79+
// because expressions can't be pushed down due to #1471.
80+
// https://github.com/opensearch-project/sql/issues/1471
81+
@Override
82+
public Boolean visitSort(Sort node, Object context) {
83+
return node.getSortList().stream().allMatch(f -> f.getField() instanceof QualifiedName
84+
&& visitField(f, context))
85+
&& canPaginate(node, context);
86+
}
87+
7988
// For queries with WHERE clause:
8089
@Override
8190
public Boolean visitFilter(Filter node, Object context) {
@@ -88,19 +97,13 @@ public Boolean visitAggregation(Aggregation node, Object context) {
8897
return Boolean.FALSE;
8998
}
9099

91-
// Queries with ORDER BY clause are not supported
92-
@Override
93-
public Boolean visitSort(Sort node, Object context) {
94-
return Boolean.FALSE;
95-
}
96-
97-
// Queries without FROM clause are not supported
100+
// For queries without FROM clause:
98101
@Override
99102
public Boolean visitValues(Values node, Object context) {
100-
return Boolean.FALSE;
103+
return Boolean.TRUE;
101104
}
102105

103-
// Queries with LIMIT clause are not supported
106+
// Queries with LIMIT/OFFSET clauses are unsupported
104107
@Override
105108
public Boolean visitLimit(Limit node, Object context) {
106109
return Boolean.FALSE;

‎core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.opensearch.sql.planner.logical.LogicalFilter;
1515
import org.opensearch.sql.planner.logical.LogicalLimit;
1616
import org.opensearch.sql.planner.logical.LogicalNested;
17+
import org.opensearch.sql.planner.logical.LogicalPaginate;
1718
import org.opensearch.sql.planner.logical.LogicalPlan;
1819
import org.opensearch.sql.planner.logical.LogicalPlanNodeVisitor;
1920
import org.opensearch.sql.planner.logical.LogicalProject;
@@ -161,6 +162,12 @@ public PhysicalPlan visitCloseCursor(LogicalCloseCursor node, C context) {
161162
return new CursorCloseOperator(visitChild(node, context));
162163
}
163164

165+
// Called when paging query requested without `FROM` clause only
166+
@Override
167+
public PhysicalPlan visitPaginate(LogicalPaginate plan, C context) {
168+
return visitChild(plan, context);
169+
}
170+
164171
protected PhysicalPlan visitChild(LogicalPlan node, C context) {
165172
// Logical operators visited here must have a single child
166173
return node.getChild().get(0).accept(this, context);

‎core/src/test/java/org/opensearch/sql/executor/pagination/CanPaginateVisitorTest.java‎

Lines changed: 67 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
import org.opensearch.sql.ast.expression.Alias;
5858
import org.opensearch.sql.ast.expression.Argument;
5959
import org.opensearch.sql.ast.expression.DataType;
60+
import org.opensearch.sql.ast.expression.Field;
6061
import org.opensearch.sql.ast.expression.Literal;
6162
import org.opensearch.sql.ast.expression.RelevanceFieldList;
6263
import org.opensearch.sql.ast.expression.UnresolvedExpression;
@@ -119,10 +120,10 @@ public void allow_query_with_select_fields_and_from() {
119120

120121
@Test
121122
// select x
122-
public void reject_query_without_from() {
123+
public void allow_query_without_from() {
123124
var plan = project(values(List.of(intLiteral(1))),
124125
alias("1", intLiteral(1)));
125-
assertFalse(plan.accept(visitor, null));
126+
assertTrue(plan.accept(visitor, null));
126127
}
127128

128129
@Test
@@ -165,63 +166,63 @@ public List<? extends Node> getChild() {
165166
public void visitEqualTo() {
166167
var plan = project(values(List.of(intLiteral(1))),
167168
alias("1", equalTo(intLiteral(1), intLiteral(1))));
168-
assertFalse(plan.accept(visitor, null));
169+
assertTrue(plan.accept(visitor, null));
169170
}
170171

171172
@Test
172173
// select interval
173174
public void visitInterval() {
174175
var plan = project(values(List.of(intLiteral(1))),
175176
alias("1", intervalLiteral(intLiteral(1), DataType.INTEGER, "days")));
176-
assertFalse(plan.accept(visitor, null));
177+
assertTrue(plan.accept(visitor, null));
177178
}
178179

179180
@Test
180181
// select a != b
181182
public void visitCompare() {
182183
var plan = project(values(List.of(intLiteral(1))),
183184
alias("1", compare("!=", intLiteral(1), intLiteral(1))));
184-
assertFalse(plan.accept(visitor, null));
185+
assertTrue(plan.accept(visitor, null));
185186
}
186187

187188
@Test
188189
// select NOT a
189190
public void visitNot() {
190191
var plan = project(values(List.of(intLiteral(1))),
191192
alias("1", not(booleanLiteral(true))));
192-
assertFalse(plan.accept(visitor, null));
193+
assertTrue(plan.accept(visitor, null));
193194
}
194195

195196
@Test
196197
// select a OR b
197198
public void visitOr() {
198199
var plan = project(values(List.of(intLiteral(1))),
199200
alias("1", or(booleanLiteral(true), booleanLiteral(false))));
200-
assertFalse(plan.accept(visitor, null));
201+
assertTrue(plan.accept(visitor, null));
201202
}
202203

203204
@Test
204205
// select a AND b
205206
public void visitAnd() {
206207
var plan = project(values(List.of(intLiteral(1))),
207208
alias("1", and(booleanLiteral(true), booleanLiteral(false))));
208-
assertFalse(plan.accept(visitor, null));
209+
assertTrue(plan.accept(visitor, null));
209210
}
210211

211212
@Test
212213
// select a XOR b
213214
public void visitXor() {
214215
var plan = project(values(List.of(intLiteral(1))),
215216
alias("1", xor(booleanLiteral(true), booleanLiteral(false))));
216-
assertFalse(plan.accept(visitor, null));
217+
assertTrue(plan.accept(visitor, null));
217218
}
218219

219220
@Test
220221
// select f()
221222
public void visitFunction() {
222223
var plan = project(values(List.of(intLiteral(1))),
223224
function("func"));
224-
assertFalse(plan.accept(visitor, null));
225+
assertTrue(plan.accept(visitor, null));
225226
}
226227

227228
@Test
@@ -237,7 +238,7 @@ public void visitNested() {
237238
public void visitIn() {
238239
// test combinations of acceptable and not acceptable args for coverage
239240
assertAll(
240-
() -> assertFalse(project(values(List.of(intLiteral(1))), alias("1", in(field("a"))))
241+
() -> assertTrue(project(values(List.of(intLiteral(1))), alias("1", in(field("a"))))
241242
.accept(visitor, null)),
242243
() -> assertFalse(project(values(List.of(intLiteral(1))),
243244
alias("1", in(field("a"), map("1", "2"))))
@@ -253,23 +254,23 @@ public void visitIn() {
253254
public void visitBetween() {
254255
var plan = project(values(List.of(intLiteral(1))),
255256
alias("1", between(field("a"), intLiteral(1), intLiteral(2))));
256-
assertFalse(plan.accept(visitor, null));
257+
assertTrue(plan.accept(visitor, null));
257258
}
258259

259260
@Test
260261
// select a CASE 1 WHEN 2
261262
public void visitCase() {
262263
var plan = project(values(List.of(intLiteral(1))),
263264
alias("1", caseWhen(intLiteral(1), when(intLiteral(3), intLiteral(4)))));
264-
assertFalse(plan.accept(visitor, null));
265+
assertTrue(plan.accept(visitor, null));
265266
}
266267

267268
@Test
268269
// select CAST(a as TYPE)
269270
public void visitCast() {
270271
// test combinations of acceptable and not acceptable args for coverage
271272
assertAll(
272-
() -> assertFalse(project(values(List.of(intLiteral(1))),
273+
() -> assertTrue(project(values(List.of(intLiteral(1))),
273274
alias("1", cast(intLiteral(2), stringLiteral("int"))))
274275
.accept(visitor, null)),
275276
() -> assertFalse(project(values(List.of(intLiteral(1))),
@@ -323,8 +324,28 @@ public void accept_query_with_highlight_and_relevance_func() {
323324

324325
@Test
325326
// select * from y limit z
326-
public void reject_query_with_limit() {
327-
var plan = project(limit(relation("dummy"), 1, 2), allFields());
327+
public void reject_query_with_limit_but_no_offset() {
328+
var plan = project(limit(relation("dummy"), 1, 0), allFields());
329+
assertFalse(plan.accept(visitor, null));
330+
}
331+
332+
@Test
333+
// select * from y limit z, n
334+
public void reject_query_with_offset() {
335+
var plan = project(limit(relation("dummy"), 0, 1), allFields());
336+
assertFalse(plan.accept(visitor, null));
337+
}
338+
339+
// test added for coverage only
340+
@Test
341+
public void visitLimit() {
342+
var visitor = new CanPaginateVisitor() {
343+
@Override
344+
public Boolean visitRelation(Relation node, Object context) {
345+
return Boolean.FALSE;
346+
}
347+
};
348+
var plan = project(limit(relation("dummy"), 0, 0), allFields());
328349
assertFalse(plan.accept(visitor, null));
329350
}
330351

@@ -338,12 +359,40 @@ public void allow_query_with_where() {
338359

339360
@Test
340361
// select * from y order by z
341-
public void reject_query_with_order_by() {
342-
var plan = project(sort(relation("dummy"), field("1")),
362+
public void allow_query_with_order_by_with_column_references_only() {
363+
var plan = project(sort(relation("dummy"), field("1")), allFields());
364+
assertTrue(plan.accept(visitor, null));
365+
}
366+
367+
@Test
368+
// select * from y order by func(z)
369+
public void reject_query_with_order_by_with_an_expression() {
370+
var plan = project(sort(relation("dummy"), field(function("func"))),
343371
allFields());
344372
assertFalse(plan.accept(visitor, null));
345373
}
346374

375+
// test added for coverage only
376+
@Test
377+
public void visitSort() {
378+
CanPaginateVisitor visitor = new CanPaginateVisitor() {
379+
@Override
380+
public Boolean visitRelation(Relation node, Object context) {
381+
return Boolean.FALSE;
382+
}
383+
};
384+
var plan = project(sort(relation("dummy"), field("1")), allFields());
385+
assertFalse(plan.accept(visitor, null));
386+
visitor = new CanPaginateVisitor() {
387+
@Override
388+
public Boolean visitField(Field node, Object context) {
389+
return Boolean.FALSE;
390+
}
391+
};
392+
plan = project(sort(relation("dummy"), field("1")), allFields());
393+
assertFalse(plan.accept(visitor, null));
394+
}
395+
347396
@Test
348397
// select * from y group by z
349398
public void reject_query_with_group_by() {
@@ -396,22 +445,6 @@ public void reject_project_when_relation_has_child() {
396445
assertFalse(visitor.visitProject((Project) plan, null));
397446
}
398447

399-
@Test
400-
// test combinations of acceptable and not acceptable args for coverage
401-
public void canPaginate() {
402-
assertAll(
403-
() -> assertFalse(project(values(List.of(intLiteral(1))),
404-
function("func", intLiteral(1), intLiteral(1)))
405-
.accept(visitor, null)),
406-
() -> assertFalse(project(values(List.of(intLiteral(1))),
407-
function("func", intLiteral(1), map("1", "2")))
408-
.accept(visitor, null)),
409-
() -> assertFalse(project(values(List.of(intLiteral(1))),
410-
function("func", map("1", "2"), intLiteral(1)))
411-
.accept(visitor, null))
412-
);
413-
}
414-
415448
@Test
416449
// test combinations of acceptable and not acceptable args for coverage
417450
public void visitFilter() {

‎core/src/test/java/org/opensearch/sql/planner/DefaultImplementorTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,17 @@
5858
import org.opensearch.sql.expression.window.WindowDefinition;
5959
import org.opensearch.sql.expression.window.ranking.RowNumberFunction;
6060
import org.opensearch.sql.planner.logical.LogicalCloseCursor;
61+
import org.opensearch.sql.planner.logical.LogicalPaginate;
6162
import org.opensearch.sql.planner.logical.LogicalPlan;
6263
import org.opensearch.sql.planner.logical.LogicalPlanDSL;
64+
import org.opensearch.sql.planner.logical.LogicalProject;
6365
import org.opensearch.sql.planner.logical.LogicalRelation;
66+
import org.opensearch.sql.planner.logical.LogicalValues;
6467
import org.opensearch.sql.planner.physical.CursorCloseOperator;
6568
import org.opensearch.sql.planner.physical.PhysicalPlan;
6669
import org.opensearch.sql.planner.physical.PhysicalPlanDSL;
70+
import org.opensearch.sql.planner.physical.ProjectOperator;
71+
import org.opensearch.sql.planner.physical.ValuesOperator;
6772
import org.opensearch.sql.storage.StorageEngine;
6873
import org.opensearch.sql.storage.Table;
6974
import org.opensearch.sql.storage.TableScanOperator;
@@ -273,4 +278,14 @@ public void visitCloseCursor_should_build_CursorCloseOperator() {
273278
assertTrue(implemented instanceof CursorCloseOperator);
274279
assertSame(physicalChild, implemented.getChild().get(0));
275280
}
281+
282+
@Test
283+
public void visitPaginate_should_remove_it_from_tree() {
284+
var logicalPlanTree = new LogicalPaginate(42, List.of(
285+
new LogicalProject(
286+
new LogicalValues(List.of(List.of())), List.of(), List.of())));
287+
var physicalPlanTree = new ProjectOperator(
288+
new ValuesOperator(List.of(List.of())), List.of(), List.of());
289+
assertEquals(physicalPlanTree, logicalPlanTree.accept(implementor, null));
290+
}
276291
}

‎core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanTestBase.java‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88

99
import com.google.common.collect.ImmutableList;
1010
import com.google.common.collect.ImmutableMap;
11+
import java.io.IOException;
12+
import java.io.ObjectInput;
13+
import java.io.ObjectOutput;
1114
import java.util.Iterator;
1215
import java.util.List;
1316
import java.util.Map;
@@ -22,6 +25,7 @@
2225
import org.opensearch.sql.expression.Expression;
2326
import org.opensearch.sql.expression.ReferenceExpression;
2427
import org.opensearch.sql.expression.env.Environment;
28+
import org.opensearch.sql.planner.SerializablePlan;
2529

2630
public class PhysicalPlanTestBase {
2731

@@ -208,7 +212,7 @@ protected static PhysicalPlan testScan(List<ExprValue> inputs) {
208212
return new TestScan(inputs);
209213
}
210214

211-
protected static class TestScan extends PhysicalPlan {
215+
protected static class TestScan extends PhysicalPlan implements SerializablePlan {
212216
private final Iterator<ExprValue> iterator;
213217

214218
public TestScan() {
@@ -238,5 +242,21 @@ public boolean hasNext() {
238242
public ExprValue next() {
239243
return iterator.next();
240244
}
245+
246+
@Override
247+
public void readExternal(ObjectInput in) throws IOException, ClassNotFoundException {
248+
}
249+
250+
@Override
251+
public void writeExternal(ObjectOutput out) throws IOException {
252+
}
253+
254+
public boolean equals(final Object o) {
255+
return o == this || o.hashCode() == hashCode();
256+
}
257+
258+
public int hashCode() {
259+
return 42;
260+
}
241261
}
242262
}

0 commit comments

Comments
 (0)