alex-plekhanov commented on code in PR #13311:
URL: https://github.com/apache/ignite/pull/13311#discussion_r3638854345
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitNode.java:
##########
@@ -85,38 +90,46 @@ public LimitNode(
/** {@inheritDoc} */
@Override public void push(Row row) throws Exception {
- if (waiting == -1)
+ if (waiting == NOT_WAITING)
return;
- ++rowsProcessed;
-
--waiting;
- checkState();
-
- if (rowsProcessed > offset) {
- if (fetchNode == null || (fetchNode != null && rowsProcessed <=
fetch + offset))
- downstream().push(row);
+ if (rowsProcessed >= offset && hasMoreData()) {
+ // this two rows can`t be swapped, cause if all requested rows
have been pushed it will trigger further request call.
Review Comment:
Comment should be started with an uppercase letter.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/IgniteSqlValidator.java:
##########
@@ -241,54 +241,81 @@ private void validateTableModify(SqlNode table) {
/** {@inheritDoc} */
@Override protected void validateSelect(SqlSelect select, RelDataType
targetRowType) {
- checkIntegerLimit(select.getFetch(), "fetch / limit");
- checkIntegerLimit(select.getOffset(), "offset");
-
super.validateSelect(select, targetRowType);
- }
- /** {@inheritDoc} */
- @Override protected void validateNamespace(SqlValidatorNamespace
namespace, RelDataType targetRowType) {
- SqlValidatorTable table = namespace.getTable();
-
- if (table != null) {
- IgniteCacheTable igniteTable =
table.unwrap(IgniteCacheTable.class);
-
- if (igniteTable != null)
- igniteTable.ensureCacheStarted();
- }
-
- super.validateNamespace(namespace, targetRowType);
+ invalidateFetchOffset(select.getFetch(), "fetch / limit");
+ invalidateFetchOffset(select.getOffset(), "offset");
}
/**
- * @param n Node to check limit.
+ * Invalidate fetch/offset params restrictions.
+ *
+ * @param n Node to check limit.
* @param nodeName Node name.
*/
- private void checkIntegerLimit(SqlNode n, String nodeName) {
+ private void invalidateFetchOffset(@Nullable SqlNode n, String nodeName) {
+ if (n == null) {
Review Comment:
Redundant braces
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitNode.java:
##########
@@ -17,29 +17,31 @@
package org.apache.ignite.internal.processors.query.calcite.exec.rel;
-import java.util.function.Supplier;
import org.apache.calcite.rel.type.RelDataType;
import
org.apache.ignite.internal.processors.query.calcite.exec.ExecutionContext;
+import org.apache.ignite.internal.processors.query.calcite.util.IgniteMath;
import org.apache.ignite.internal.util.typedef.F;
-import org.jetbrains.annotations.Nullable;
/** Offset, fetch|limit support node. */
public class LimitNode<Row> extends AbstractNode<Row> implements
SingleNode<Row>, Downstream<Row> {
- /** Offset if its present, otherwise 0. */
- private final int offset;
+ /** Offset param. */
+ private final long offset;
- /** Fetch if its present, otherwise 0. */
- private final int fetch;
+ /** Fetch param. */
+ private final long fetch;
+
+ /** Fetch can be unset. */
+ private final boolean fetchUndefined;
Review Comment:
Let's add `long limit = fetch == -1 ? Long.MAX_VALUE :
IgniteMath.addExact(fetch, offset)` instead. To execute
`IgniteMath.addExact(fetch, offset)` only once (and not for each request and
each push), to simplify `hasMoreData` (`rowsProcessed < limit`).
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitNode.java:
##########
@@ -17,29 +17,31 @@
package org.apache.ignite.internal.processors.query.calcite.exec.rel;
-import java.util.function.Supplier;
import org.apache.calcite.rel.type.RelDataType;
import
org.apache.ignite.internal.processors.query.calcite.exec.ExecutionContext;
+import org.apache.ignite.internal.processors.query.calcite.util.IgniteMath;
import org.apache.ignite.internal.util.typedef.F;
-import org.jetbrains.annotations.Nullable;
/** Offset, fetch|limit support node. */
public class LimitNode<Row> extends AbstractNode<Row> implements
SingleNode<Row>, Downstream<Row> {
- /** Offset if its present, otherwise 0. */
- private final int offset;
+ /** Offset param. */
+ private final long offset;
- /** Fetch if its present, otherwise 0. */
- private final int fetch;
+ /** Fetch param. */
+ private final long fetch;
+
+ /** Fetch can be unset. */
+ private final boolean fetchUndefined;
/** Already processed (pushed to upstream) rows count. */
private int rowsProcessed;
Review Comment:
Comparing int with long, can overflow
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/IgniteSqlValidator.java:
##########
@@ -241,54 +241,81 @@ private void validateTableModify(SqlNode table) {
/** {@inheritDoc} */
@Override protected void validateSelect(SqlSelect select, RelDataType
targetRowType) {
- checkIntegerLimit(select.getFetch(), "fetch / limit");
- checkIntegerLimit(select.getOffset(), "offset");
-
super.validateSelect(select, targetRowType);
- }
- /** {@inheritDoc} */
- @Override protected void validateNamespace(SqlValidatorNamespace
namespace, RelDataType targetRowType) {
- SqlValidatorTable table = namespace.getTable();
-
- if (table != null) {
- IgniteCacheTable igniteTable =
table.unwrap(IgniteCacheTable.class);
-
- if (igniteTable != null)
- igniteTable.ensureCacheStarted();
- }
-
- super.validateNamespace(namespace, targetRowType);
+ invalidateFetchOffset(select.getFetch(), "fetch / limit");
+ invalidateFetchOffset(select.getOffset(), "offset");
}
/**
- * @param n Node to check limit.
+ * Invalidate fetch/offset params restrictions.
+ *
+ * @param n Node to check limit.
* @param nodeName Node name.
*/
- private void checkIntegerLimit(SqlNode n, String nodeName) {
+ private void invalidateFetchOffset(@Nullable SqlNode n, String nodeName) {
Review Comment:
invalidate - means make something invalid. Here we validate parameters,
let's use `validate`or `check`.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitExecutionTest.java:
##########
@@ -109,56 +108,21 @@ private void checkLimit(int offset, int fetch) {
RelDataType rowType = TypeUtils.createRowType(tf, int.class);
RootNode<Object[]> rootNode = new RootNode<>(ctx, rowType);
- LimitNode<Object[]> limitNode = new LimitNode<>(ctx, rowType, () ->
offset, fetch == 0 ? null : () -> fetch);
- SourceNode srcNode = new SourceNode(ctx, rowType);
+ LimitNode<Object[]> limitNode = new LimitNode<>(ctx, rowType, offset,
fetch == 0 ? -1 : fetch);
+
+ List<Object[]> data = IntStream.range(0, IN_BUFFER_SIZE + fetch +
offset).boxed()
+ .map(i -> new Object[] {i}).collect(Collectors.toList());
+
+ ScanNode<Object[]> srcNode = new ScanNode<>(ctx, rowType, data);
rootNode.register(limitNode);
limitNode.register(srcNode);
- if (fetch > 0) {
- for (int i = offset; i < offset + fetch; i++) {
- assertTrue(rootNode.hasNext());
- assertEquals(i, rootNode.next()[0]);
- }
-
- assertFalse(rootNode.hasNext());
- assertEquals(srcNode.requested.get(), offset + fetch);
- }
- else {
+ for (int i = offset; i < offset + fetch; i++) {
assertTrue(rootNode.hasNext());
- assertEquals(offset, rootNode.next()[0]);
Review Comment:
This check for case `fetch == 0` was removed to
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/LimitExecutionTest.java:
##########
@@ -109,56 +108,21 @@ private void checkLimit(int offset, int fetch) {
RelDataType rowType = TypeUtils.createRowType(tf, int.class);
RootNode<Object[]> rootNode = new RootNode<>(ctx, rowType);
- LimitNode<Object[]> limitNode = new LimitNode<>(ctx, rowType, () ->
offset, fetch == 0 ? null : () -> fetch);
- SourceNode srcNode = new SourceNode(ctx, rowType);
+ LimitNode<Object[]> limitNode = new LimitNode<>(ctx, rowType, offset,
fetch == 0 ? -1 : fetch);
+
+ List<Object[]> data = IntStream.range(0, IN_BUFFER_SIZE + fetch +
offset).boxed()
+ .map(i -> new Object[] {i}).collect(Collectors.toList());
+
+ ScanNode<Object[]> srcNode = new ScanNode<>(ctx, rowType, data);
rootNode.register(limitNode);
limitNode.register(srcNode);
- if (fetch > 0) {
- for (int i = offset; i < offset + fetch; i++) {
- assertTrue(rootNode.hasNext());
- assertEquals(i, rootNode.next()[0]);
- }
-
- assertFalse(rootNode.hasNext());
- assertEquals(srcNode.requested.get(), offset + fetch);
Review Comment:
This check is crucial, it shows that we don't request redundant rows from
source node. Why it was removed?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -438,14 +470,31 @@ protected static TestTable createTable(IgniteSchema
schema, String name, RelData
return table;
}
+ /** */
+ protected <T extends RelNode> void assertPlan(
+ TestPlanningContextBuilder ctxBuilder
+ ) throws Exception {
+ assertPlan(ctxBuilder, rel -> true);
+ }
+
+ /** */
+ protected <T extends RelNode> void assertPlan(
+ TestPlanningContextBuilder ctxBuilder,
+ Predicate<T> predicate
+ ) throws Exception {
+ invalidatePlan(ctxBuilder, predicate);
Review Comment:
Why do we need this method (it just a wrapper for invalidatePlan)
Why `invalidatePlan`? We don't invalidate anything, we check plans (better
to use assertPlan too)
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -805,4 +857,79 @@ class TestFailureProcessor extends FailureProcessor {
return true;
}
}
+
+ /** Test planning context builder. */
+ public static class TestPlanningContextBuilder {
+ /** */
+ private String query;
+
+ /** */
+ private Collection<IgniteSchema> schemas;
+
+ /** */
+ private Collection<Object> params = List.of();
+
+ /** */
+ private Collection<String> disabledRules = List.of();
+
+ /** */
+ @Nullable private RelOptListener planListener;
+
+ /** */
+ public TestPlanningContextBuilder query(String qry) {
+ query = qry;
+ return this;
+ }
+
+ /** */
+ public TestPlanningContextBuilder schema(IgniteSchema schemas) {
+ this.schemas = List.of(schemas);
+ return this;
+ }
+
+ /** */
+ public TestPlanningContextBuilder schemas(Collection<IgniteSchema>
schemas) {
+ this.schemas = List.copyOf(schemas);
+ return this;
+ }
+
+ /** */
+ public TestPlanningContextBuilder params(Collection<Object> params) {
+ this.params = List.copyOf(params);
+ return this;
+ }
+
+ /** */
+ public TestPlanningContextBuilder params(Object... params) {
+ this.params = Arrays.asList(params);
+ return this;
+ }
+
+ /** */
+ public TestPlanningContextBuilder disabledRules(String... rules) {
+ disabledRules = List.of(rules);
+ return this;
+ }
+
+ /** */
+ public TestPlanningContextBuilder planListener(@Nullable
RelOptListener planListener) {
+ this.planListener = planListener;
+ return this;
+ }
+ }
+
+ /** */
+ public static TestPlanningContextBuilder contextBuilder() {
+ return new TestPlanningContextBuilder();
+ }
+
+ /** */
+ @SuppressWarnings("ThrowableNotThrown")
+ static void assertThrows(
Review Comment:
Let's rewrite to assertThrows(PlannerContext, cls, msg) or
assertThrows(PlannerContextBuilder, cls, msg) and use physicalPlan(...) method
inside (not assertPlan)
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java:
##########
@@ -101,40 +98,6 @@ public void testNestedLimitOffsetWithUnion() {
).returns(2).returns(4).check();
}
- /** Tests correctness of fetch / offset params. */
- @Test
- public void testInvalidLimitOffset() {
- String bigInt = BigDecimal.valueOf(10000000000L).toString();
-
- assertThrows("SELECT * FROM TEST_REPL OFFSET " + bigInt + " ROWS",
- SqlValidatorException.class, "Illegal value of offset");
-
- assertThrows("SELECT * FROM TEST_REPL FETCH FIRST " + bigInt + " ROWS
ONLY",
- SqlValidatorException.class, "Illegal value of fetch / limit");
-
- assertThrows("SELECT * FROM TEST_REPL LIMIT " + bigInt,
- SqlValidatorException.class, "Illegal value of fetch / limit");
-
- assertThrows("SELECT * FROM TEST_REPL OFFSET -1 ROWS FETCH FIRST -1
ROWS ONLY",
- IgniteSQLException.class, null);
-
- assertThrows("SELECT * FROM TEST_REPL OFFSET -1 ROWS",
- IgniteSQLException.class, null);
-
- assertThrows("SELECT * FROM TEST_REPL OFFSET 2+1 ROWS",
- IgniteSQLException.class, null);
-
- // Check with parameters
- assertThrows("SELECT * FROM TEST_REPL OFFSET ? ROWS FETCH FIRST ? ROWS
ONLY",
- SqlValidatorException.class, "Illegal value of fetch / limit", -1,
-1);
-
- assertThrows("SELECT * FROM TEST_REPL OFFSET ? ROWS",
- SqlValidatorException.class, "Illegal value of offset", -1);
-
- assertThrows("SELECT * FROM TEST_REPL FETCH FIRST ? ROWS ONLY",
- SqlValidatorException.class, "Illegal value of fetch / limit", -1);
Review Comment:
Looks like these checks are still valid, why they was removed?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -220,20 +221,51 @@ protected PlanningContext plannerCtx(
@Nullable RelOptListener planLsnr,
String... disabledRules
) {
- return plannerCtx(sql, Collections.singleton(publicSchema), planLsnr,
disabledRules);
+ return plannerCtx(sql, Collections.singleton(publicSchema), planLsnr,
null, disabledRules);
+ }
+
+ /** */
+ private PlanningContext plannerCtx(
Review Comment:
Fully duplicated another method except one parameter `disabledRules`. Let's
unify these methods.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -455,20 +504,34 @@ protected <T extends RelNode> void assertPlan(
Predicate<T> predicate,
String... disabledRules
) throws Exception {
- assertPlan(sql, schemas, null, predicate, disabledRules);
+ TestPlanningContextBuilder builder =
contextBuilder().query(sql).schemas(schemas).disabledRules(disabledRules);
+
+ assertPlan(builder, predicate);
}
/** */
protected <T extends RelNode> void assertPlan(
String sql,
- Collection<IgniteSchema> schemas,
- @Nullable RelOptListener planLsnr,
+ IgniteSchema schema,
+ RelOptListener planLsnr,
Predicate<T> predicate,
String... disabledRules
) throws Exception {
- IgniteRel plan = physicalPlan(plannerCtx(sql, schemas, planLsnr,
disabledRules));
+ TestPlanningContextBuilder builder =
contextBuilder().query(sql).schema(schema).disabledRules(disabledRules)
+ .planListener(planLsnr);
+
+ assertPlan(builder, predicate);
+ }
+
+ /** */
+ private <T extends RelNode> void invalidatePlan(
+ TestPlanningContextBuilder ctxBuilder,
+ Predicate<T> predicate
+ ) throws Exception {
+ IgniteRel plan = physicalPlan(plannerCtx(ctxBuilder.query,
ctxBuilder.schemas, ctxBuilder.planListener,
Review Comment:
Move `plannerCtx(...)` to `TestPlanningContextBuilder.build()` method.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/HashJoinPlannerTest.java:
##########
@@ -28,10 +28,10 @@
import
org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
import org.apache.ignite.internal.util.typedef.F;
import org.apache.ignite.internal.util.typedef.internal.CU;
+import org.apache.ignite.testframework.GridTestUtils;
Review Comment:
Redundant change
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -58,21 +57,21 @@ public class SortNode<Row> extends MemoryTrackingNode<Row>
implements SingleNode
public SortNode(
ExecutionContext<Row> ctx, RelDataType rowType,
Comparator<Row> comp,
- @Nullable Supplier<Integer> offset,
- @Nullable Supplier<Integer> fetch
+ long offset,
+ long fetch
) {
super(ctx, rowType);
- assert fetch == null || fetch.get() >= 0;
- assert offset == null || offset.get() >= 0;
+ assert fetch == -1 || fetch >= 0;
+ assert offset >= 0;
- limit = fetch == null ? -1 : fetch.get() + (offset == null ? 0 :
offset.get());
+ limit = fetch == -1 ? -1 : (fetch > Long.MAX_VALUE - offset ? -1 :
fetch + offset);
- if (limit < 0)
+ if (limit < 1 || limit > Integer.MAX_VALUE)
Review Comment:
Can we rich this line with limit == 0? If yes, it's more correct to use
`limit <= 1' or add other logic to return empty result
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -438,14 +470,31 @@ protected static TestTable createTable(IgniteSchema
schema, String name, RelData
return table;
}
+ /** */
+ protected <T extends RelNode> void assertPlan(
Review Comment:
Redundant method. Used in assertThrows, but it's better to write own
assertThrows method with parameters like PlannerContext or
PlannerContextBuilder.
--
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]
