You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/05/13 13:05:29 UTC
[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #9987: IGNITE-16013 : Calcite engine. Introduce sort-with-limit physical relational operator
alex-plekhanov commented on code in PR #9987:
URL: https://github.com/apache/ignite/pull/9987#discussion_r872373253
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/DataTypesTest.java:
##########
@@ -35,6 +35,18 @@
* Test SQL data types.
*/
public class DataTypesTest extends AbstractBasicIntegrationTest {
+ /** TODO. */
+ @Test
+ public void testTest() {
Review Comment:
?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteSort.java:
##########
@@ -92,9 +127,13 @@ public IgniteSort(RelInput input) {
if (isEnforcer() || required.getConvention() != IgniteConvention.INSTANCE)
return null;
- RelCollation collation = TraitUtils.collation(required);
+ RelCollation requiredCollation = TraitUtils.collation(required);
+ RelCollation relCollation = traitSet.getCollation();
- return Pair.of(required.replace(collation), ImmutableList.of(required.replace(RelCollations.EMPTY)));
+ if (!requiredCollation.satisfies(relCollation))
+ return null;
+
+ return Pair.of(required.replace(requiredCollation), ImmutableList.of(required.replace(RelCollations.EMPTY)));
Review Comment:
`required.replace(requiredCollation)` -> `required`
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitExecutionTest.java:
##########
@@ -48,6 +52,59 @@ public void testLimit() throws Exception {
checkLimit(2000, 3000);
}
+ /** Tests Sort node can limit its output when fetch param is set. */
+ @Test
+ public void testSortLimit() throws Exception {
+ int bufSize = U.field(AbstractNode.class, "IN_BUFFER_SIZE");
+
+ checkLimitSort(0, 1);
+ checkLimitSort(1, 0);
+ checkLimitSort(1, 1);
+ checkLimitSort(0, bufSize);
+ checkLimitSort(bufSize, 0);
+ checkLimitSort(bufSize, bufSize);
+ checkLimitSort(bufSize - 1, 1);
+ checkLimitSort(2000, 0);
+ checkLimitSort(0, 3000);
+ checkLimitSort(2000, 3000);
+ }
+
+ /**
+ * @param offset Rows offset.
+ * @param fetch Fetch rows count (zero means unlimited).
+ */
+ private void checkLimitSort(int offset, int fetch) {
+ assert offset >= 0;
+ assert fetch >= 0;
+
+ ExecutionContext<Object[]> ctx = executionContext(F.first(nodes()), UUID.randomUUID(), 0);
+ IgniteTypeFactory tf = ctx.getTypeFactory();
+ RelDataType rowType = TypeUtils.createRowType(tf, int.class);
+
+ RootNode<Object[]> rootNode = new RootNode<>(ctx, rowType);
+
+ SortNode<Object[]> sortNode = new SortNode<>(ctx, rowType, F::compareArrays, () -> offset,
+ fetch == 0 ? null : () -> fetch);
+
+ List<Object[]> data = IntStream.range(0, SourceNode.IN_BUFFER_SIZE + fetch + offset).boxed()
+ .map(i -> new Object[] {i}).collect(Collectors.toList());
+ Collections.shuffle(data);
+
+ ScanNode<Object[]> srcNode = new ScanNode<>(ctx, rowType, data);
+
+ rootNode.register(sortNode);
+
+ sortNode.register(srcNode);
+
+ for (int i = 0; i < offset + fetch; i++) {
+ assertTrue(rootNode.hasNext());
+ assertEquals(i, rootNode.next()[0]);
+ }
+
+ if (fetch > 0)
+ assertFalse(rootNode.hasNext());
Review Comment:
```
assertEquals(fetch == 0, rootNode.hasNext());
```
?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,21 +43,53 @@
/** Rows buffer. */
private final PriorityQueue<Row> rows;
+ /** SQL select limit. Negative if disabled. */
+ private final int limit;
+
+ /** Reverse-ordered rows in case of limited sort. */
+ private List<Row> reversed;
+
/**
* @param ctx Execution context.
* @param comp Rows comparator.
+ * @param offset Offset.
+ * @param fetch Limit.
*/
- public SortNode(ExecutionContext<Row> ctx, RelDataType rowType, Comparator<Row> comp) {
+ public SortNode(
+ ExecutionContext<Row> ctx, RelDataType rowType,
+ Comparator<Row> comp,
+ @Nullable Supplier<Integer> offset,
+ @Nullable Supplier<Integer> fetch
+ ) {
super(ctx, rowType);
- rows = comp == null ? new PriorityQueue<>() : new PriorityQueue<>(comp);
+ assert fetch == null || fetch.get() >= 0;
+ assert offset == null || offset.get() >= 0;
+
+ limit = fetch == null ? -1 : fetch.get() + (offset == null ? 0 : offset.get());
+
+ if (limit < 0)
+ rows = new PriorityQueue<>(comp);
+ else
+ rows = new GridBoundedPriorityQueue<>(limit, comp == null ? (Comparator<Row>)Comparator.reverseOrder()
+ : comp.reversed());
Review Comment:
`{}` required here according to codestyle
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/LimitOffsetPlannerTest.java:
##########
@@ -82,31 +84,53 @@ public void testOrderOfRels() throws Exception {
IgniteSchema publicSchema = createSchemaWithTable(IgniteDistributions.random());
// Simple case, Limit can't be pushed down under Exchange or Sort. Sort before Exchange is more preferable.
- assertPlan("SELECT * FROM TEST ORDER BY ID LIMIT 10 OFFSET 10", publicSchema,
+ assertPlan("SELECT * FROM TEST ORDER BY ID LIMIT 5 OFFSET 10", publicSchema,
+ isInstanceOf(IgniteLimit.class)
+ .and(input(isInstanceOf(IgniteExchange.class)
+ .and(input(isInstanceOf(IgniteSort.class)
+ .and(s -> doubleFromRex(s.fetch, -1) == 5.0)
+ .and(s -> doubleFromRex(s.offset, -1) == 10.0))))));
+
+ // Same simple case but witout offset.
+ assertPlan("SELECT * FROM TEST ORDER BY ID LIMIT 5", publicSchema,
+ isInstanceOf(IgniteLimit.class)
+ .and(input(isInstanceOf(IgniteExchange.class)
+ .and(input(isInstanceOf(IgniteSort.class)
+ .and(s -> doubleFromRex(s.fetch, -1) == 5.0)
+ .and(s -> s.offset == null))))));
+
+ // No special liited sort required if LIMIT is not set.
+ assertPlan("SELECT * FROM TEST ORDER BY ID OFFSET 10", publicSchema,
isInstanceOf(IgniteLimit.class)
.and(input(isInstanceOf(IgniteExchange.class)
- .and(input(isInstanceOf(IgniteSort.class))))));
+ .and(input(isInstanceOf(IgniteSort.class)
+ .and(s -> s.fetch == null)
+ .and(s -> s.offset == null))))));
// Simple case without ordering.
- assertPlan("SELECT * FROM TEST OFFSET 10 ROWS FETCH FIRST 10 ROWS ONLY", publicSchema,
+ assertPlan("SELECT * FROM TEST OFFSET 10 ROWS FETCH FIRST 5 ROWS ONLY", publicSchema,
isInstanceOf(IgniteLimit.class)
- .and(input(isInstanceOf(IgniteExchange.class)))
- .and(hasChildThat(isInstanceOf(IgniteSort.class)).negate()));
+ .and(s -> doubleFromRex(s.fetch(), -1) == 5)
+ .and(s -> doubleFromRex(s.offset(), -1) == 10)
+ .and(input(isInstanceOf(IgniteExchange.class)))
Review Comment:
Wrong indent (should be on the same level as doubleFromRex check)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org