You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ignite.apache.org by zs...@apache.org on 2025/04/11 05:41:13 UTC
(ignite-3) branch main updated: IGNITE-24227 Sql. Improve type checking in LIMIT / OFFSET clauses (#5479)
This is an automated email from the ASF dual-hosted git repository.
zstan pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/ignite-3.git
The following commit(s) were added to refs/heads/main by this push:
new d15a709de2a IGNITE-24227 Sql. Improve type checking in LIMIT / OFFSET clauses (#5479)
d15a709de2a is described below
commit d15a709de2a96a2458a9660678a1fe3474ee8a92
Author: Evgeniy Stanilovskiy <st...@gmail.com>
AuthorDate: Fri Apr 11 08:41:07 2025 +0300
IGNITE-24227 Sql. Improve type checking in LIMIT / OFFSET clauses (#5479)
---
.../sql/engine/ItDynamicParameterTest.java | 4 ++
.../internal/sql/engine/ItLimitOffsetTest.java | 42 ++++++++----
.../sql/group1/order/test_limit.test | 24 +++++++
.../sql/group1/order/test_offset.test | 14 ++++
.../sql/engine/exec/LogicalRelImplementor.java | 49 +++++++-------
.../sql/engine/exec/exp/ScalarImplementor.java | 11 ++--
.../internal/sql/engine/exec/rel/LimitNode.java | 75 +++++++++++++---------
.../internal/sql/engine/exec/rel/SortNode.java | 22 +++----
.../sql/engine/prepare/IgniteSqlValidator.java | 46 +++++++------
.../internal/sql/engine/util/IgniteResource.java | 4 +-
.../sql/engine/exec/rel/ExchangeExecutionTest.java | 2 +-
.../sql/engine/exec/rel/LimitExecutionTest.java | 64 ++++--------------
.../sql/engine/planner/DynamicParametersTest.java | 16 +++--
13 files changed, 212 insertions(+), 161 deletions(-)
diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
index c43fc362b85..5692066d6ac 100644
--- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
+++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
@@ -33,6 +33,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.params.provider.Arguments.arguments;
+import java.math.BigDecimal;
import java.sql.Date;
import java.time.LocalDate;
import java.time.ZoneOffset;
@@ -189,6 +190,9 @@ public class ItDynamicParameterTest extends BaseSqlIntegrationTest {
assertQuery("SELECT name LIKE '%' || ? || '%' FROM person where name is not null").withParams("go").returns(true).returns(false)
.returns(false).returns(false).check();
+ assertQuery("SELECT id FROM person ORDER BY id LIMIT ?").withParams(1).returns(0).check();
+ assertQuery("SELECT id FROM person ORDER BY id LIMIT ?").withParams(new BigDecimal(1)).returns(0).check();
+
assertQuery("SELECT id FROM person WHERE name LIKE ? ORDER BY id LIMIT ?").withParams("I%", 1).returns(0).check();
assertQuery("SELECT id FROM person WHERE name LIKE ? ORDER BY id LIMIT ? OFFSET ?").withParams("I%", 1, 1).returns(2).check();
assertQuery("SELECT id from person WHERE salary<? and id<?").withParams(15, 3).returns(0).check();
diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
index a2339909135..4b23f945794 100644
--- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
+++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
@@ -47,16 +47,37 @@ public class ItLimitOffsetTest extends BaseSqlIntegrationTest {
/** Tests correctness of fetch / offset params. */
@Test
public void testInvalidLimitOffset() {
- String bigInt = BigDecimal.valueOf(10000000000L).toString();
+ BigDecimal moreThanUpperLong = new BigDecimal(Long.MAX_VALUE).add(new BigDecimal(1));
+
+ // cache the plan with concrete type param
+ igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", new BigDecimal(Long.MAX_VALUE));
+
+ assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", new BigDecimal(-1)));
+
+ assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", -1));
+
+ assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", (Object) null));
+
+ assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", moreThanUpperLong));
+
+ assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", (double) Long.MAX_VALUE + 1));
+
+ assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", (float) Long.MAX_VALUE + 1));
assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
- () -> igniteSql().execute(null, "SELECT * FROM test OFFSET " + bigInt + " ROWS"));
+ () -> igniteSql().execute(null, "SELECT * FROM test OFFSET " + moreThanUpperLong + " ROWS"));
assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of fetch / limit",
- () -> igniteSql().execute(null, "SELECT * FROM test FETCH FIRST " + bigInt + " ROWS ONLY"));
+ () -> igniteSql().execute(null, "SELECT * FROM test FETCH FIRST " + moreThanUpperLong + " ROWS ONLY"));
assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of fetch / limit",
- () -> igniteSql().execute(null, "SELECT * FROM test LIMIT " + bigInt));
+ () -> igniteSql().execute(null, "SELECT * FROM test LIMIT " + moreThanUpperLong));
assertThrowsSqlException(Sql.STMT_PARSE_ERR,
"Failed to parse query: Encountered \"-\"",
@@ -66,19 +87,12 @@ public class ItLimitOffsetTest extends BaseSqlIntegrationTest {
"Failed to parse query: Encountered \"-\"",
() -> igniteSql().execute(null, "SELECT * FROM test OFFSET -1 ROWS"));
+ assertThrowsSqlException(Sql.STMT_PARSE_ERR, "Failed to parse query: Encountered \"-\"",
+ () -> igniteSql().execute(null, "SELECT * FROM test FETCH FIRST -1 ROWS ONLY"));
+
assertThrowsSqlException(Sql.STMT_PARSE_ERR,
"Failed to parse query: Encountered \"+\"",
() -> igniteSql().execute(null, "SELECT * FROM test OFFSET 2+1 ROWS"));
-
- // Check with parameters
- assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of fetch / limit",
- () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS FETCH FIRST ? ROWS ONLY", -1, -1));
-
- assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of offset",
- () -> igniteSql().execute(null, "SELECT * FROM test OFFSET ? ROWS", -1));
-
- assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, "Illegal value of fetch / limit",
- () -> igniteSql().execute(null, "SELECT * FROM test FETCH FIRST ? ROWS ONLY", -1));
}
/**
diff --git a/modules/sql-engine/src/integrationTest/sql/group1/order/test_limit.test b/modules/sql-engine/src/integrationTest/sql/group1/order/test_limit.test
index 2c2c3d34db9..bc2f2f1261c 100644
--- a/modules/sql-engine/src/integrationTest/sql/group1/order/test_limit.test
+++ b/modules/sql-engine/src/integrationTest/sql/group1/order/test_limit.test
@@ -17,6 +17,30 @@ SELECT a FROM test ORDER BY a LIMIT 1
----
11
+# decimal limit
+query I
+SELECT a FROM test ORDER BY a LIMIT 1.2
+----
+11
+
+# decimal limit
+query I
+SELECT a FROM test ORDER BY a FETCH FIRST 1.2 ROWS ONLY
+----
+11
+
+# decimal limit
+query I
+SELECT a FROM test ORDER BY a LIMIT 1.6
+----
+11
+
+# decimal limit
+query I
+SELECT a FROM test ORDER BY a FETCH FIRST 1.6 ROWS ONLY
+----
+11
+
# LIMIT with non-scalar should fail
statement error
SELECT a FROM test LIMIT a
diff --git a/modules/sql-engine/src/integrationTest/sql/group1/order/test_offset.test b/modules/sql-engine/src/integrationTest/sql/group1/order/test_offset.test
index 929ce38608c..21fa8d725d5 100644
--- a/modules/sql-engine/src/integrationTest/sql/group1/order/test_offset.test
+++ b/modules/sql-engine/src/integrationTest/sql/group1/order/test_offset.test
@@ -18,6 +18,20 @@ SELECT b FROM test ORDER BY b DESC OFFSET 1
7
2
+# decimal offset
+query I
+SELECT b FROM test ORDER BY b DESC OFFSET 1.4
+----
+7
+2
+
+# decimal offset
+query I
+SELECT b FROM test ORDER BY b DESC OFFSET 1.6
+----
+7
+2
+
query I
SELECT b FROM test ORDER BY b OFFSET 2
----
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/LogicalRelImplementor.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/LogicalRelImplementor.java
index 379a9fd9755..81556d31dc9 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/LogicalRelImplementor.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/LogicalRelImplementor.java
@@ -147,7 +147,11 @@ import org.apache.ignite.internal.sql.engine.trait.IgniteDistribution;
import org.apache.ignite.internal.sql.engine.trait.TraitUtils;
import org.apache.ignite.internal.sql.engine.type.IgniteTypeFactory;
import org.apache.ignite.internal.sql.engine.util.Commons;
+import org.apache.ignite.internal.sql.engine.util.IgniteMath;
+import org.apache.ignite.internal.sql.engine.util.IgniteResource;
import org.apache.ignite.internal.util.IgniteUtils;
+import org.apache.ignite.lang.ErrorGroups.Sql;
+import org.apache.ignite.sql.SqlException;
import org.jetbrains.annotations.Nullable;
/**
@@ -647,17 +651,8 @@ public class LogicalRelImplementor<RowT> implements IgniteRelVisitor<Node<RowT>>
/** {@inheritDoc} */
@Override
public Node<RowT> visit(IgniteLimit rel) {
- Supplier<Integer> offset = null;
- if (rel.offset() != null) {
- SqlScalar<RowT, Integer> sqlScalar = expressionFactory.scalar(rel.offset());
- offset = () -> sqlScalar.get(ctx);
- }
-
- Supplier<Integer> fetch = null;
- if (rel.fetch() != null) {
- SqlScalar<RowT, Integer> sqlScalar = expressionFactory.scalar(rel.fetch());
- fetch = () -> sqlScalar.get(ctx);
- }
+ long offset = rel.offset() == null ? 0 : validateAndGetFetchOffsetParams(rel.offset(), "offset");
+ long fetch = rel.fetch() == null ? -1 : validateAndGetFetchOffsetParams(rel.fetch(), "fetch");
LimitNode<RowT> node = new LimitNode<>(ctx, offset, fetch);
@@ -673,17 +668,8 @@ public class LogicalRelImplementor<RowT> implements IgniteRelVisitor<Node<RowT>>
public Node<RowT> visit(IgniteSort rel) {
RelCollation collation = rel.getCollation();
- Supplier<Integer> offset = null;
- if (rel.offset != null) {
- SqlScalar<RowT, Integer> sqlScalar = expressionFactory.scalar(rel.offset);
- offset = () -> sqlScalar.get(ctx);
- }
-
- Supplier<Integer> fetch = null;
- if (rel.fetch != null) {
- SqlScalar<RowT, Integer> sqlScalar = expressionFactory.scalar(rel.fetch);
- fetch = () -> sqlScalar.get(ctx);
- }
+ long offset = rel.offset == null ? 0 : validateAndGetFetchOffsetParams(rel.offset, "offset");
+ long fetch = rel.fetch == null ? -1 : validateAndGetFetchOffsetParams(rel.fetch, "fetch");
SqlComparator<RowT> sqlComparator = expressionFactory.comparator(collation);
SortNode<RowT> node = new SortNode<>(
@@ -1168,6 +1154,25 @@ public class LogicalRelImplementor<RowT> implements IgniteRelVisitor<Node<RowT>>
return joinProjection;
}
+ private long validateAndGetFetchOffsetParams(RexNode node, String op) {
+ SqlScalar<RowT, Number> sqlScalar = expressionFactory.scalar(node);
+ Number param = sqlScalar.get(ctx);
+
+ long paramAsLong;
+
+ try {
+ paramAsLong = IgniteMath.convertToLongExact(param);
+ } catch (RuntimeException ex) {
+ throw new SqlException(Sql.STMT_VALIDATION_ERR, IgniteResource.INSTANCE.illegalFetchLimit(op).str(), ex);
+ }
+
+ if (paramAsLong < 0) {
+ throw new SqlException(Sql.STMT_VALIDATION_ERR, IgniteResource.INSTANCE.illegalFetchLimit(op).str());
+ }
+
+ return paramAsLong;
+ }
+
/**
* Evaluate a null comparison strategy as a bitset for the case of NOT DISTINCT FROM syntax.
*/
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/ScalarImplementor.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/ScalarImplementor.java
index ff16b875b1f..a6d03ede540 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/ScalarImplementor.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/ScalarImplementor.java
@@ -22,7 +22,6 @@ import static org.apache.ignite.internal.sql.engine.util.Commons.cast;
import java.lang.reflect.Modifier;
import java.util.List;
-import org.apache.calcite.DataContext;
import org.apache.calcite.adapter.java.JavaTypeFactory;
import org.apache.calcite.linq4j.function.Function1;
import org.apache.calcite.linq4j.tree.BlockBuilder;
@@ -33,6 +32,7 @@ import org.apache.calcite.linq4j.tree.ParameterExpression;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexDynamicParam;
import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.rex.RexProgram;
@@ -104,14 +104,11 @@ class ScalarImplementor {
ParameterExpression ctx = Expressions.parameter(ExecutionContext.class, "ctx");
- builder.add(
- Expressions.declare(Modifier.FINAL, DataContext.ROOT, Expressions.convert_(ctx, DataContext.class))
- );
-
Expression rowHandler = builder.append("hnd", Expressions.call(ctx, IgniteMethod.CONTEXT_ROW_HANDLER.method()));
- Function1<String, InputGetter> correlates = new CorrelatesBuilder(builder, ctx, rowHandler)
- .build(List.of(scalarValue));
+ Function1<String, InputGetter> correlates = scalarValue instanceof RexDynamicParam
+ ? null
+ : new CorrelatesBuilder(builder, ctx, rowHandler).build(List.of(scalarValue));
List<Expression> projects = RexToLixTranslator.translateProjects(program, typeFactory, conformance,
builder, null, null, ctx, NoOpFieldGetter.INSTANCE, correlates);
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitNode.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitNode.java
index ba94e3db7cf..a777456d34e 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitNode.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitNode.java
@@ -19,27 +19,29 @@ package org.apache.ignite.internal.sql.engine.exec.rel;
import static org.apache.ignite.internal.util.CollectionUtils.nullOrEmpty;
-import java.util.function.Supplier;
import org.apache.ignite.internal.sql.engine.exec.ExecutionContext;
-import org.jetbrains.annotations.Nullable;
+import org.apache.ignite.internal.sql.engine.util.IgniteMath;
/** Offset, fetch|limit support node. */
public class LimitNode<RowT> extends AbstractNode<RowT> implements SingleNode<RowT>, Downstream<RowT> {
- /** 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;
- /** Already processed (pushed to upstream) rows count. */
- private int rowsProcessed;
+ /** Fetch can be unset. */
+ private final boolean fetchUndefined;
- /** Fetch can be unset, in this case we need all rows. */
- private @Nullable Supplier<Integer> fetchNode;
+ /** Already processed (pushed to upstream) rows count. */
+ private long rowsProcessed;
/** Waiting results counter. */
private int waiting;
+ /** Upper requested rows. */
+ private int requested;
+
/**
* Constructor.
*
@@ -47,14 +49,14 @@ public class LimitNode<RowT> extends AbstractNode<RowT> implements SingleNode<Ro
*/
public LimitNode(
ExecutionContext<RowT> ctx,
- Supplier<Integer> offsetNode,
- Supplier<Integer> fetchNode
+ long offset,
+ long fetch
) {
super(ctx);
- offset = offsetNode == null ? 0 : offsetNode.get();
- fetch = fetchNode == null ? 0 : fetchNode.get();
- this.fetchNode = fetchNode;
+ this.offset = offset;
+ fetchUndefined = fetch == -1;
+ this.fetch = fetch == -1 ? 0 : fetch;
}
/** {@inheritDoc} */
@@ -63,22 +65,24 @@ public class LimitNode<RowT> extends AbstractNode<RowT> implements SingleNode<Ro
assert !nullOrEmpty(sources()) && sources().size() == 1;
assert rowsCnt > 0;
- if (fetchNone()) {
+ if (!hasMoreData()) {
end();
return;
}
- if (offset > 0 && rowsProcessed == 0) {
- rowsCnt = offset + rowsCnt;
- }
-
- waiting = rowsCnt;
+ assert requested == 0 : requested;
+ requested = rowsCnt;
if (fetch > 0) {
- rowsCnt = Math.min(rowsCnt, (fetch + offset) - rowsProcessed);
+ long remain = IgniteMath.addExact(fetch, offset) - rowsProcessed;
+
+ rowsCnt = remain > rowsCnt ? rowsCnt : (int) remain;
}
+ waiting = rowsCnt;
+
+ checkState();
source().request(rowsCnt);
}
@@ -89,19 +93,30 @@ public class LimitNode<RowT> extends AbstractNode<RowT> implements SingleNode<Ro
if (waiting == NOT_WAITING) {
return;
}
- ++rowsProcessed;
--waiting;
- if (rowsProcessed > offset) {
- if (fetchNode == null || rowsProcessed <= fetch + offset) {
+ if (rowsProcessed >= offset) {
+ if (hasMoreData()) {
+ // this two rows can`t be swapped, cause if all requested rows have been pushed it will trigger further request call.
+ --requested;
downstream().push(row);
}
}
- if (fetch > 0 && rowsProcessed == fetch + offset && waiting > 0) {
+ ++rowsProcessed;
+
+ // There several cases are possible:
+ // 1) requested = 512, limit = 1, offset = not defined: need to pass 1 row and call end()
+ // 2) requested = 512, limit = 512, offset = not defined: just need to pass all rows without end() call
+ // 3) requested = 512, limit = 512, offset = 1: need to request initially 512 and further 1 row
+ if (!hasMoreData() && requested > 0) {
end();
}
+
+ if (waiting == 0 && requested > 0) {
+ source().request(waiting = requested);
+ }
}
/** {@inheritDoc} */
@@ -121,6 +136,8 @@ public class LimitNode<RowT> extends AbstractNode<RowT> implements SingleNode<Ro
/** {@inheritDoc} */
@Override
protected void rewindInternal() {
+ waiting = 0;
+ requested = 0;
rowsProcessed = 0;
}
@@ -134,8 +151,8 @@ public class LimitNode<RowT> extends AbstractNode<RowT> implements SingleNode<Ro
return this;
}
- /** {@code True} if requested 0 results, or all already processed. */
- private boolean fetchNone() {
- return (fetchNode != null && fetch == 0) || (fetch > 0 && rowsProcessed == fetch + offset);
+ /** {@code True} if fetch is undefined, or current rows processed is less than required. */
+ private boolean hasMoreData() {
+ return fetchUndefined || rowsProcessed < fetch + offset;
}
}
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/SortNode.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/SortNode.java
index d5bf4591a2d..528dd31e428 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/SortNode.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/SortNode.java
@@ -23,10 +23,9 @@ import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.PriorityQueue;
-import java.util.function.Supplier;
import org.apache.ignite.internal.sql.engine.exec.ExecutionContext;
+import org.apache.ignite.internal.sql.engine.util.IgniteMath;
import org.apache.ignite.internal.util.BoundedPriorityQueue;
-import org.jetbrains.annotations.Nullable;
/**
* Sort node.
@@ -44,7 +43,7 @@ public class SortNode<RowT> extends AbstractNode<RowT> implements SingleNode<Row
private final PriorityQueue<RowT> rows;
/** SQL select limit. Negative if disabled. */
- private final int limit;
+ private final long limit;
/** Reverse-ordered rows in case of limited sort. */
private List<RowT> reversed;
@@ -59,18 +58,19 @@ public class SortNode<RowT> extends AbstractNode<RowT> implements SingleNode<Row
*/
public SortNode(ExecutionContext<RowT> ctx,
Comparator<RowT> comp,
- @Nullable Supplier<Integer> offset,
- @Nullable Supplier<Integer> fetch) {
+ long offset,
+ long fetch) {
super(ctx);
- assert fetch == null || fetch.get() >= 0;
- assert offset == null || offset.get() >= 0;
- limit = fetch == null ? -1 : fetch.get() + (offset == null ? 0 : offset.get());
+ assert fetch == -1 || fetch >= 0;
+ assert offset >= 0;
- if (limit < 1) {
+ limit = fetch == -1 ? -1 : IgniteMath.addExact(fetch, offset);
+
+ if (limit < 1 || limit > Integer.MAX_VALUE) {
rows = new PriorityQueue<>(comp);
} else {
- rows = new BoundedPriorityQueue<>(limit, comp == null ? (Comparator<RowT>) Comparator.reverseOrder() : comp.reversed());
+ rows = new BoundedPriorityQueue<>((int) limit, comp == null ? (Comparator<RowT>) Comparator.reverseOrder() : comp.reversed());
}
}
@@ -81,7 +81,7 @@ public class SortNode<RowT> extends AbstractNode<RowT> implements SingleNode<Row
* @param comp Rows comparator.
*/
public SortNode(ExecutionContext<RowT> ctx, Comparator<RowT> comp) {
- this(ctx, comp, null, null);
+ this(ctx, comp, 0, -1);
}
/** {@inheritDoc} */
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
index 7d601bc51d0..1829acfec02 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
@@ -120,8 +120,8 @@ import org.jetbrains.annotations.Nullable;
/** Validator. */
public class IgniteSqlValidator extends SqlValidatorImpl {
- /** Decimal of Integer.MAX_VALUE for fetch/offset bounding. */
- private static final BigDecimal DEC_INT_MAX = BigDecimal.valueOf(Integer.MAX_VALUE);
+ /** Decimal of Long.MAX_VALUE for fetch/offset bounding. */
+ public static final BigDecimal LIMIT_UPPER = BigDecimal.valueOf(Long.MAX_VALUE);
public static final int MAX_LENGTH_OF_ALIASES = 256;
public static final int DECIMAL_DYNAMIC_PARAM_PRECISION = 28;
@@ -701,51 +701,55 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
protected void validateSelect(SqlSelect select, RelDataType targetRowType) {
super.validateSelect(select, targetRowType);
- checkIntegerLimit(select.getFetch(), "fetch / limit");
- checkIntegerLimit(select.getOffset(), "offset");
+ invalidateFetchOffset(select.getFetch(), "fetch / limit");
+ invalidateFetchOffset(select.getOffset(), "offset");
}
/**
- * Check integer limit.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
+ * Invalidate fetch/offset params restrictions.
*
* @param n Node to check limit.
* @param nodeName Node name.
*/
- private void checkIntegerLimit(@Nullable SqlNode n, String nodeName) {
+ private void invalidateFetchOffset(@Nullable SqlNode n, String nodeName) {
if (n == null) {
return;
}
if (n instanceof SqlLiteral) {
- BigDecimal offFetchLimit = ((SqlLiteral) n).bigDecimalValue();
+ BigDecimal offsetFetchLimit = ((SqlLiteral) n).bigDecimalValue();
- if (offFetchLimit.compareTo(DEC_INT_MAX) > 0 || offFetchLimit.compareTo(BigDecimal.ZERO) < 0) {
- throw newValidationError(n, IgniteResource.INSTANCE.correctIntegerLimit(nodeName));
- }
+ checkLimitOffset(offsetFetchLimit, n, nodeName);
} else if (n instanceof SqlDynamicParam) {
SqlDynamicParam dynamicParam = (SqlDynamicParam) n;
- RelDataType intType = typeFactory.createSqlType(INTEGER);
+ SqlTypeName expectType = SqlTypeName.BIGINT;
+ RelDataType dynParamType = typeFactory.createSqlType(expectType);
// Validate value, if present.
if (!isUnspecified(dynamicParam)) {
Object param = getDynamicParamValue(dynamicParam);
- if (param instanceof Integer) {
- if ((Integer) param < 0) {
- throw newValidationError(n, IgniteResource.INSTANCE.correctIntegerLimit(nodeName));
- }
- } else {
- String actualType = deriveDynamicParamType(dynamicParam).toString();
- String expectedType = intType.toString();
+ if (param == null) {
+ throw newValidationError(n, IgniteResource.INSTANCE.illegalFetchLimit(nodeName));
+ }
- var err = IgniteResource.INSTANCE.incorrectDynamicParameterType(expectedType, actualType);
+ dynParamType = deriveDynamicParamType(dynamicParam);
+
+ if (!SqlTypeUtil.isNumeric(dynParamType)) {
+ var err = IgniteResource.INSTANCE.incorrectDynamicParameterType(expectType.toString(),
+ dynParamType.getSqlTypeName().toString());
throw newValidationError(n, err);
}
}
// Dynamic parameters are nullable.
- setDynamicParamType(dynamicParam, typeFactory.createTypeWithNullability(intType, true));
+ setDynamicParamType(dynamicParam, typeFactory.createTypeWithNullability(dynParamType, true));
+ }
+ }
+
+ private void checkLimitOffset(BigDecimal offsetFetchLimit, @Nullable SqlNode n, String nodeName) {
+ if (offsetFetchLimit.compareTo(LIMIT_UPPER) > 0 || offsetFetchLimit.signum() == -1) {
+ throw newValidationError(n, IgniteResource.INSTANCE.illegalFetchLimit(nodeName));
}
}
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
index 4be54fd7d3f..54029aa695d 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
@@ -49,8 +49,8 @@ public interface IgniteResource {
@BaseMessage("Illegal aggregate function. {0} is unsupported at the moment")
ExInst<SqlValidatorException> unsupportedAggregationFunction(String a0);
- @BaseMessage("Illegal value of {0}. The value must be positive and less than Integer.MAX_VALUE (" + Integer.MAX_VALUE + ")")
- ExInst<SqlValidatorException> correctIntegerLimit(String a0);
+ @BaseMessage("Illegal value of {0}. The value must be positive and less than " + Long.MAX_VALUE)
+ ExInst<SqlValidatorException> illegalFetchLimit(String a0);
@BaseMessage("Invalid decimal literal")
ExInst<SqlValidatorException> decimalLiteralInvalid();
diff --git a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/ExchangeExecutionTest.java b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/ExchangeExecutionTest.java
index b119b371c8b..833add825d5 100644
--- a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/ExchangeExecutionTest.java
+++ b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/ExchangeExecutionTest.java
@@ -524,7 +524,7 @@ public class ExchangeExecutionTest extends AbstractExecutionTest<Object[]> {
AbstractNode<Object[]> node = inbox;
if (limit > 0) {
- node = new LimitNode<>(targetCtx, () -> 0, () -> 1);
+ node = new LimitNode<>(targetCtx, 0, 1);
node.register(List.of(inbox));
}
diff --git a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitExecutionTest.java b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitExecutionTest.java
index b726beaa890..a085c3abb43 100644
--- a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitExecutionTest.java
+++ b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/rel/LimitExecutionTest.java
@@ -19,12 +19,10 @@ package org.apache.ignite.internal.sql.engine.exec.rel;
import static org.apache.ignite.internal.sql.engine.util.Commons.IN_BUFFER_SIZE;
import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Collections;
import java.util.List;
-import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import org.apache.ignite.internal.sql.engine.exec.ExecutionContext;
@@ -59,7 +57,7 @@ public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
/** Tests Sort node can limit its output when fetch param is set. */
@Test
- public void testSortLimit() throws Exception {
+ public void testSortLimit() {
int bufSize = IN_BUFFER_SIZE;
checkLimitSort(0, 1);
@@ -88,8 +86,8 @@ public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
RootNode<Object[]> rootNode = new RootNode<>(ctx);
- SortNode<Object[]> sortNode = new SortNode<>(ctx, LimitExecutionTest::compareArrays, () -> offset,
- fetch == 0 ? null : () -> fetch);
+ SortNode<Object[]> sortNode = new SortNode<>(ctx, LimitExecutionTest::compareArrays, 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());
@@ -119,25 +117,21 @@ public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
ExecutionContext<Object[]> ctx = executionContext(true);
RootNode<Object[]> rootNode = new RootNode<>(ctx);
- LimitNode<Object[]> limitNode = new LimitNode<>(ctx, () -> offset, fetch == 0 ? null : () -> fetch);
- SourceNode srcNode = new SourceNode(ctx);
+ LimitNode<Object[]> limitNode = new LimitNode<>(ctx, 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, 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]);
- assertTrue(srcNode.requested.get() > offset);
+ assertEquals(i, rootNode.next()[0]);
}
+
+ assertEquals(fetch == 0, rootNode.hasNext());
}
/**
@@ -146,7 +140,7 @@ public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
* @param val Value to check.
* @return {@code True} if not null and array.
*/
- public static boolean isArray(Object val) {
+ private static boolean isArray(Object val) {
return val != null && val.getClass().isArray();
}
@@ -157,7 +151,7 @@ public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
* @param a2 Value 2.
* @return a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second.
*/
- public static int compareArrays(Object[] a1, Object[] a2) {
+ private static int compareArrays(Object[] a1, Object[] a2) {
if (a1 == a2) {
return 0;
}
@@ -187,36 +181,6 @@ public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
return Integer.compare(a1.length, a2.length);
}
- private static class SourceNode extends AbstractNode<Object[]> {
- AtomicInteger requested = new AtomicInteger();
-
- public SourceNode(ExecutionContext<Object[]> ctx) {
- super(ctx);
- }
-
- /** {@inheritDoc} */
- @Override
- protected void rewindInternal() {
- // No-op.
- }
-
- /** {@inheritDoc} */
- @Override protected Downstream<Object[]> requestDownstream(int idx) {
- return null;
- }
-
- /** {@inheritDoc} */
- @Override public void request(int rowsCnt) {
- int r = requested.getAndAdd(rowsCnt);
-
- this.execute(() -> {
- for (int i = 0; i < rowsCnt; i++) {
- downstream().push(new Object[]{r + i});
- }
- });
- }
- }
-
@Override
protected RowHandler<Object[]> rowHandler() {
return ArrayRowHandler.INSTANCE;
diff --git a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/DynamicParametersTest.java b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/DynamicParametersTest.java
index 25d7d208ee3..d95b1ed502c 100644
--- a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/DynamicParametersTest.java
+++ b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/DynamicParametersTest.java
@@ -724,12 +724,12 @@ public class DynamicParametersTest extends AbstractPlannerTest {
checkStatement(setup)
.sql("SELECT * FROM t1 LIMIT ?", Unspecified.UNKNOWN)
- .parameterTypes(nullable(NativeTypes.INT32))
+ .parameterTypes(nullable(NativeTypes.INT64))
.ok(),
checkStatement(setup)
.sql("SELECT * FROM t1 LIMIT ?", "a")
- .fails("Incorrect type of a dynamic parameter. Expected <INTEGER> but got <VARCHAR"),
+ .fails("Incorrect type of a dynamic parameter. Expected <BIGINT> but got <VARCHAR>"),
checkStatement(setup)
.sql("SELECT * FROM t1 OFFSET ?", 1)
@@ -738,12 +738,20 @@ public class DynamicParametersTest extends AbstractPlannerTest {
checkStatement(setup)
.sql("SELECT * FROM t1 OFFSET ?", Unspecified.UNKNOWN)
- .parameterTypes(nullable(NativeTypes.INT32))
+ .parameterTypes(nullable(NativeTypes.INT64))
.ok(),
checkStatement(setup)
.sql("SELECT * FROM t1 OFFSET ?", "a")
- .fails("Incorrect type of a dynamic parameter. Expected <INTEGER> but got <VARCHAR")
+ .fails("Incorrect type of a dynamic parameter. Expected <BIGINT> but got <VARCHAR>"),
+
+ checkStatement(setup)
+ .sql("SELECT * FROM t1 LIMIT ?", (Object) null)
+ .fails("Illegal value of fetch / limit"),
+
+ checkStatement(setup)
+ .sql("SELECT * FROM t1 OFFSET ? ROWS", (Object) null)
+ .fails("Illegal value of offset")
);
}