alex-plekhanov commented on code in PR #9987:
URL: https://github.com/apache/ignite/pull/9987#discussion_r859742077
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,21 +43,66 @@
/** 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 limit 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> limit)
{
super(ctx, rowType);
- rows = comp == null ? new PriorityQueue<>() : new
PriorityQueue<>(comp);
+ assert limit == null || limit.get() >= 0;
+ assert offset == null || offset.get() >= 0;
+
+ this.limit = limit == null ? -1 : limit.get() + (offset == null ? 0 :
offset.get());
+
+ if (this.limit < 0)
+ rows = new PriorityQueue<>(comp);
+ else {
+ Comparator<Row> reverseCmp;
+
+ // Reverse comparator
+ if (comp == null) {
+ reverseCmp = new Comparator<Row>() {
Review Comment:
`Comparator.reverseOrder()`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,21 +43,66 @@
/** 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 limit 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> limit)
{
Review Comment:
Let's rename `limit` to `fetch` to fit `Sort` rel node fields
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java:
##########
@@ -41,10 +41,10 @@
/** */
public class IgniteLimit extends SingleRel implements IgniteRel {
/** In case the fetch value is a DYNAMIC_PARAM. */
- private static final double FETCH_IS_PARAM_FACTOR = 0.01;
+ static final double FETCH_IS_PARAM_FACTOR = 0.01;
/** In case the offset value is a DYNAMIC_PARAM. */
- private static final double OFFSET_IS_PARAM_FACTOR = 0.5;
+ static final double OFFSET_IS_PARAM_FACTOR = 0.5;
Review Comment:
Still not moved
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,21 +43,66 @@
/** 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 limit 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> limit)
{
super(ctx, rowType);
- rows = comp == null ? new PriorityQueue<>() : new
PriorityQueue<>(comp);
+ assert limit == null || limit.get() >= 0;
+ assert offset == null || offset.get() >= 0;
+
+ this.limit = limit == null ? -1 : limit.get() + (offset == null ? 0 :
offset.get());
+
+ if (this.limit < 0)
+ rows = new PriorityQueue<>(comp);
+ else {
+ Comparator<Row> reverseCmp;
+
+ // Reverse comparator
Review Comment:
Point at the end.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,21 +43,66 @@
/** 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 limit 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> limit)
{
super(ctx, rowType);
- rows = comp == null ? new PriorityQueue<>() : new
PriorityQueue<>(comp);
+ assert limit == null || limit.get() >= 0;
+ assert offset == null || offset.get() >= 0;
+
+ this.limit = limit == null ? -1 : limit.get() + (offset == null ? 0 :
offset.get());
+
+ if (this.limit < 0)
+ rows = new PriorityQueue<>(comp);
+ else {
+ Comparator<Row> reverseCmp;
+
+ // Reverse comparator
+ if (comp == null) {
+ reverseCmp = new Comparator<Row>() {
+ @Override public int compare(Row r1, Row r2) {
+ return ((Comparable<Row>)r2).compareTo(r1);
+ }
+ };
+ }
+ else {
+ reverseCmp = new Comparator<Row>() {
Review Comment:
`comp.reversed()`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,21 +43,66 @@
/** 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 limit Limit.
*/
- public SortNode(ExecutionContext<Row> ctx, RelDataType rowType,
Comparator<Row> comp) {
+ public SortNode(ExecutionContext<Row> ctx, RelDataType rowType,
Comparator<Row> comp,
Review Comment:
Each parameter should be on it's own line
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteSort.java:
##########
@@ -127,6 +156,20 @@ public IgniteSort(RelInput input) {
/** {@inheritDoc} */
@Override public IgniteRel clone(RelOptCluster cluster, List<IgniteRel>
inputs) {
- return new IgniteSort(cluster, getTraitSet(), sole(inputs), collation);
+ return new IgniteSort(cluster, getTraitSet(), sole(inputs), collation,
offset, fetch);
+ }
+
+ /** {@inheritDoc} */
+ @Override public boolean isEnforcer() {
+ return true;
Review Comment:
Why it's always true? When we convert sort explicitly - it's not an enforcer
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java:
##########
@@ -41,10 +41,10 @@
/** */
public class IgniteLimit extends SingleRel implements IgniteRel {
/** In case the fetch value is a DYNAMIC_PARAM. */
- private static final double FETCH_IS_PARAM_FACTOR = 0.01;
+ static final double FETCH_IS_PARAM_FACTOR = 0.01;
Review Comment:
Still not moved
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/LimitOffsetTest.java:
##########
@@ -35,15 +35,14 @@
import org.apache.ignite.internal.util.typedef.F;
import org.apache.ignite.internal.util.typedef.X;
import org.apache.ignite.internal.util.typedef.internal.U;
-import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.junit.Test;
import static java.util.Collections.singletonList;
/**
* Limit / offset tests.
*/
-public class LimitOffsetTest extends GridCommonAbstractTest {
+public class LimitOffsetTest extends AbstractBasicIntegrationTest {
Review Comment:
Any new integration tests? Or all cases already covered by existing tests? I
think we need at least execution node test (see `ExecutionTestSuite`). If there
are no new tests in this class, file shouldn't be changed.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -116,14 +166,22 @@ private void flush() throws Exception {
int processed = 0;
+ if (limit > 0 && reversed == null && !rows.isEmpty()) {
Review Comment:
Let's move it inside `inLoop` section
Let's process rows using batches to allow others to do their job
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteSort.java:
##########
@@ -107,16 +128,24 @@ public IgniteSort(RelInput input) {
return Pair.of(childTraits.replace(collation()),
ImmutableList.of(childTraits));
}
+ /** {@inheritDoc} */
+ @Override public double estimateRowCount(RelMetadataQuery mq) {
+ return memRows(super.estimateRowCount(mq));
Review Comment:
I think it's safer to use `mq.getRowCount(getInput())` but not rely on super
method, since it can be overridden by `SortNode` at some point in time and will
produce the wrong result.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/LimitOffsetPlannerTest.java:
##########
@@ -40,6 +41,24 @@ public class LimitOffsetPlannerTest extends
AbstractPlannerTest {
/** Row count in table. */
private static final double ROW_CNT = 100d;
+ /** Tests Exchange goes before Sort and limit. */
+ @Test
+ public void testLimitExchange() throws Exception {
Review Comment:
What the difference between checks in this test and checks in
`testOrderOfRels`?
Shouldn't we additionally check `offset` and `fetch` fields of `IgniteSort`
node?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
