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 ab2c07a553 IGNITE-18437 Sql. Fix row count estimate by limit with
offset - Fixes #1463.
ab2c07a553 is described below
commit ab2c07a553910738f7e90ac9b7eff7c6cf13ac69
Author: zstan <[email protected]>
AuthorDate: Mon Dec 26 16:37:05 2022 +0300
IGNITE-18437 Sql. Fix row count estimate by limit with offset - Fixes #1463.
Signed-off-by: zstan <[email protected]>
---
.../internal/sql/engine/ItLimitOffsetTest.java | 210 +++++++++++++++++++++
.../internal/sql/engine/rel/IgniteLimit.java | 19 +-
.../sql/engine/exec/MockedStructuresTest.java | 2 +-
.../sql/engine/exec/rel/LimitExecutionTest.java | 5 +-
.../sql/engine/planner/LimitOffsetPlannerTest.java | 6 +
5 files changed, 227 insertions(+), 15 deletions(-)
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
new file mode 100644
index 0000000000..0867f61114
--- /dev/null
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
@@ -0,0 +1,210 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.sql.engine;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.ignite.internal.sql.engine.util.Commons;
+import org.apache.ignite.lang.IgniteException;
+import org.apache.ignite.sql.IgniteSql;
+import org.apache.ignite.sql.Session;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Check LIMIT and\or OFFSET commands.
+ */
+public class ItLimitOffsetTest extends AbstractBasicIntegrationTest {
+ @BeforeEach
+ void beforeEach() {
+ sql("CREATE TABLE test (pk INT PRIMARY KEY, col0 INT)");
+ }
+
+ @AfterEach
+ void afterEach() {
+ sql("DROP TABLE IF EXISTS test");
+ }
+
+ protected IgniteSql igniteSql() {
+ return CLUSTER_NODES.get(0).sql();
+ }
+
+ /** Tests correctness of fetch / offset params. */
+ @Test
+ public void testInvalidLimitOffset() {
+ Session session = igniteSql().createSession();
+
+ String bigInt = BigDecimal.valueOf(10000000000L).toString();
+
+ //todo: correct exception
https://issues.apache.org/jira/browse/IGNITE-16095, here and all checks near.
+ IgniteException ret = assertThrows(IgniteException.class, ()
+ -> session.execute(null, "SELECT * FROM test OFFSET " + bigInt
+ " ROWS"));
+ assertTrue(ret.getMessage().contains("Illegal value of offset"));
+
+ ret = assertThrows(IgniteException.class,
+ () -> session.execute(null, "SELECT * FROM test FETCH FIRST "
+ bigInt + " ROWS ONLY"));
+ assertTrue(ret.getMessage().contains("Illegal value of fetch /
limit"));
+
+ ret = assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test LIMIT " + bigInt));
+ assertTrue(ret.getMessage().contains("Illegal value of fetch /
limit"));
+
+ assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test OFFSET -1 ROWS "
+ + "FETCH FIRST -1 ROWS ONLY"));
+
+ assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test OFFSET -1 ROWS"));
+
+ assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test OFFSET 2+1 ROWS"));
+
+ // Check with parameters
+ ret = assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test OFFSET ? "
+ + "ROWS FETCH FIRST ? ROWS ONLY", -1, -1));
+ assertTrue(ret.getMessage().contains("Illegal value of fetch /
limit"));
+
+ ret = assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test OFFSET ? ROWS", -1));
+ assertTrue(ret.getMessage().contains("Illegal value of offset"));
+
+ ret = assertThrows(IgniteException.class, () -> session.execute(null,
"SELECT * FROM test FETCH FIRST ? ROWS ONLY", -1));
+ assertTrue(ret.getMessage().contains("Illegal value of fetch /
limit"));
+ }
+
+ /**
+ * Check execution correctness.
+ */
+ @Test
+ public void testLimitOffset() {
+ int inBufSize = Commons.IN_BUFFER_SIZE;
+
+ int[] rowsArr = {10, inBufSize, (2 * inBufSize) - 1};
+
+ for (int rows : rowsArr) {
+ List<List<Object>> res = sql("SELECT COUNT(*) FROM test");
+
+ long count = (long) res.get(0).get(0);
+
+ for (long i = count; i < rows; ++i) {
+ sql(String.format("INSERT INTO test VALUES(%d, %d);", i, i));
+ }
+
+ int[] limits = {-1, 0, 10, rows / 2 - 1, rows / 2, rows / 2 + 1,
rows - 1, rows};
+ int[] offsets = {-1, 0, 10, rows / 2 - 1, rows / 2, rows / 2 + 1,
rows - 1, rows};
+
+ for (int lim : limits) {
+ for (int off : offsets) {
+ log.info("Check [rows=" + rows + ", limit=" + lim + ",
off=" + off + ']');
+
+ checkQuery(rows, lim, off, false, false);
+ checkQuery(rows, lim, off, true, false);
+ checkQuery(rows, lim, off, false, true);
+ checkQuery(rows, lim, off, true, true);
+ }
+ }
+ }
+ }
+
+ /** Check correctness of row count estimation. */
+ @Test
+ public void testOffsetOutOfRange() {
+ for (long i = 0; i < 5; ++i) {
+ sql(String.format("INSERT INTO test VALUES(%d, %d);", i, i));
+ }
+
+ assertQuery("SELECT (SELECT pk FROM test ORDER BY pk LIMIT 1 OFFSET
10)").returns(new Object[]{null}).check();
+ }
+
+ /**
+ * Check query with specified limit and offset.
+ *
+ * @param rows Rows count.
+ * @param lim Limit.
+ * @param off Offset.
+ * @param param If {@code false} place limit/offset as literals, otherwise
they are placed as parameters.
+ * @param sorted Use sorted query (adds ORDER BY).
+ */
+ void checkQuery(int rows, int lim, int off, boolean param, boolean sorted)
{
+ String request = createSql(lim, off, param, sorted);
+
+ Object[] params = null;
+ if (lim >= 0 && off >= 0) {
+ params = new Object[]{off, lim};
+ } else if (lim >= 0) {
+ params = new Object[]{lim};
+ } else if (off >= 0) {
+ params = new Object[]{off};
+ }
+
+ log.info("SQL: " + request + (param ? "params=" +
Arrays.toString(params) : ""));
+
+ List<List<Object>> res = params != null ? sql(request, params) :
sql(request);
+
+ assertEquals(expectedSize(rows, lim, off), res.size(), "Invalid
results size. [rows=" + rows + ", limit=" + lim + ", off=" + off
+ + ", res=" + res.size() + ']');
+ }
+
+ /**
+ * Calculates expected result set size by limit and offset.
+ */
+ private int expectedSize(int rows, int lim, int off) {
+ if (off < 0) {
+ off = 0;
+ }
+
+ if (lim == 0) {
+ return 0;
+ } else if (lim < 0) {
+ return rows - off;
+ } else if (lim + off < rows) {
+ return lim;
+ } else if (off > rows) {
+ return 0;
+ } else {
+ return rows - off;
+ }
+ }
+
+ /**
+ * Form sql request according to incoming parameters.
+ *
+ * @param lim Limit.
+ * @param off Offset.
+ * @param param Flag to place limit/offset by parameter or literal.
+ * @return SQL query string.
+ */
+ private String createSql(int lim, int off, boolean param, boolean sorted) {
+ StringBuilder sb = new StringBuilder("SELECT * FROM test ");
+
+ if (sorted) {
+ sb.append("ORDER BY pk ");
+ }
+
+ if (off >= 0) {
+ sb.append("OFFSET ").append(param ? "?" :
Integer.toString(off)).append(" ROWS ");
+ }
+
+ if (lim >= 0) {
+ sb.append("FETCH FIRST ").append(param ? "?" :
Integer.toString(lim)).append(" ROWS ONLY ");
+ }
+
+ return sb.toString();
+ }
+}
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteLimit.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteLimit.java
index 03846bfb33..e01d05c5ea 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteLimit.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteLimit.java
@@ -36,10 +36,7 @@ import
org.apache.ignite.internal.sql.engine.metadata.cost.IgniteCost;
import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
import org.apache.ignite.internal.sql.engine.trait.TraitUtils;
-/**
- * IgniteLimit.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
- */
+/** Relational expression that applies a limit and/or offset to its input. */
public class IgniteLimit extends SingleRel implements InternalIgniteRel {
/** In case the fetch value is a DYNAMIC_PARAM. */
private static final double FETCH_IS_PARAM_FACTOR = 0.01;
@@ -75,8 +72,9 @@ public class IgniteLimit extends SingleRel implements
InternalIgniteRel {
}
/**
- * Constructor.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
+ * Constructor, used for deserialization purpose.
+ *
+ * @param input Input relational expression.
*/
public IgniteLimit(RelInput input) {
super(
@@ -156,12 +154,7 @@ public class IgniteLimit extends SingleRel implements
InternalIgniteRel {
/** {@inheritDoc} */
@Override
public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery
mq) {
- double inputRowCount = mq.getRowCount(getInput());
-
- double lim = fetch != null ? doubleFromRex(fetch, inputRowCount *
FETCH_IS_PARAM_FACTOR) : inputRowCount;
- double off = offset != null ? doubleFromRex(offset, inputRowCount *
OFFSET_IS_PARAM_FACTOR) : 0;
-
- double rows = Math.min(lim + off, inputRowCount);
+ double rows = estimateRowCount(mq);
return planner.getCostFactory().makeCost(rows, rows *
IgniteCost.ROW_PASS_THROUGH_COST, 0);
}
@@ -174,7 +167,7 @@ public class IgniteLimit extends SingleRel implements
InternalIgniteRel {
double lim = fetch != null ? doubleFromRex(fetch, inputRowCount *
FETCH_IS_PARAM_FACTOR) : inputRowCount;
double off = offset != null ? doubleFromRex(offset, inputRowCount *
OFFSET_IS_PARAM_FACTOR) : 0;
- return Math.min(lim, inputRowCount - off);
+ return Math.max(0, Math.min(lim, inputRowCount - off));
}
/**
diff --git
a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java
b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java
index feb8242f57..f8b8d9f5d2 100644
---
a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java
+++
b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java
@@ -280,7 +280,7 @@ public class MockedStructuresTest extends
IgniteAbstractTest {
* Tests create a table through public API.
*/
@Test
- public void testCreateTable() throws Exception {
+ public void testCreateTable() {
SqlQueryProcessor finalQueryProc = queryProc;
String curMethodName = getCurrentMethodName();
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 7cafefa920..061550601b 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
@@ -39,8 +39,12 @@ public class LimitExecutionTest extends
AbstractExecutionTest {
checkLimit(1, 0);
checkLimit(1, 1);
checkLimit(0, bufSize);
+ checkLimit(0, bufSize - 1);
+ checkLimit(0, bufSize + 1);
checkLimit(bufSize, 0);
checkLimit(bufSize, bufSize);
+ checkLimit(bufSize, bufSize - 1);
+ checkLimit(bufSize, bufSize + 1);
checkLimit(bufSize - 1, 1);
checkLimit(2000, 0);
checkLimit(0, 3000);
@@ -79,7 +83,6 @@ public class LimitExecutionTest extends AbstractExecutionTest
{
}
private static class SourceNode extends AbstractNode<Object[]> {
-
AtomicInteger requested = new AtomicInteger();
public SourceNode(ExecutionContext<Object[]> ctx) {
diff --git
a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/LimitOffsetPlannerTest.java
b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/LimitOffsetPlannerTest.java
index 2878881803..e5f219cb68 100644
---
a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/LimitOffsetPlannerTest.java
+++
b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/LimitOffsetPlannerTest.java
@@ -70,6 +70,12 @@ public class LimitOffsetPlannerTest extends
AbstractPlannerTest {
assertPlan("SELECT * FROM TEST ORDER BY ID OFFSET 60", publicSchema,
nodeOrAnyChild(isInstanceOf(IgniteLimit.class)
.and(l -> l.getCluster().getMetadataQuery().getRowCount(l)
== ROW_CNT - 60d)));
+
+ assertPlan("SELECT * FROM TEST ORDER BY ID LIMIT 1 OFFSET " + ROW_CNT
* 2, publicSchema,
+ nodeOrAnyChild(isInstanceOf(IgniteLimit.class)
+ // Estimated row count returned by IgniteLimit node is
0, but after validation it becomes 1
+ // if it less than 1.
+ .and(l ->
l.getCluster().getMetadataQuery().getRowCount(l) == 1)));
}
@Test
