This is an automated email from the ASF dual-hosted git repository. alexpl pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/ignite.git
The following commit(s) were added to refs/heads/master by this push: new fe445101265 IGNITE-18363 SQL Calcite: Fix row count estimate by limit with offset - Fixes #10428. fe445101265 is described below commit fe445101265968109c5c4de82e635ec1700f58c4 Author: Aleksey Plekhanov <plehanov.a...@gmail.com> AuthorDate: Mon Dec 12 18:00:28 2022 +0300 IGNITE-18363 SQL Calcite: Fix row count estimate by limit with offset - Fixes #10428. Signed-off-by: Aleksey Plekhanov <plehanov.a...@gmail.com> --- .../processors/query/calcite/rel/IgniteLimit.java | 15 +- .../integration/AbstractBasicIntegrationTest.java | 7 +- .../query/calcite/integration/FunctionsTest.java | 3 - .../LimitOffsetIntegrationTest.java} | 169 +++++---------------- .../calcite/planner/LimitOffsetPlannerTest.java | 6 + .../ignite/testsuites/IntegrationTestSuite.java | 4 +- 6 files changed, 57 insertions(+), 147 deletions(-) diff --git a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java index 3bd29161807..ae3f91bcad3 100644 --- a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java +++ b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java @@ -136,24 +136,19 @@ public class IgniteLimit extends SingleRel implements IgniteRel { /** {@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); } /** {@inheritDoc} */ @Override public double estimateRowCount(RelMetadataQuery mq) { - double inputRowCount = mq.getRowCount(getInput()); + double inputRowCnt = 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 lim = fetch != null ? doubleFromRex(fetch, inputRowCnt * FETCH_IS_PARAM_FACTOR) : inputRowCnt; + double off = offset != null ? doubleFromRex(offset, inputRowCnt * OFFSET_IS_PARAM_FACTOR) : 0; - return Math.min(lim, inputRowCount - off); + return Math.max(0, Math.min(lim, inputRowCnt - off)); } /** diff --git a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractBasicIntegrationTest.java b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractBasicIntegrationTest.java index 8e3a38476b7..94086d33cad 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractBasicIntegrationTest.java +++ b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractBasicIntegrationTest.java @@ -60,6 +60,9 @@ import static org.apache.ignite.testframework.GridTestUtils.waitForCondition; */ @WithSystemProperty(key = "calcite.debug", value = "false") public class AbstractBasicIntegrationTest extends GridCommonAbstractTest { + /** */ + protected static final Object[] NULL_RESULT = new Object[] { null }; + /** */ protected static final String TABLE_NAME = "person"; @@ -147,8 +150,8 @@ public class AbstractBasicIntegrationTest extends GridCommonAbstractTest { * @param cls Exception class. * @param msg Error message. */ - protected void assertThrows(String sql, Class<? extends Exception> cls, String msg) { - assertThrowsAnyCause(log, () -> executeSql(sql), cls, msg); + protected void assertThrows(String sql, Class<? extends Exception> cls, String msg, Object... args) { + assertThrowsAnyCause(log, () -> executeSql(sql, args), cls, msg); } /** */ diff --git a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/FunctionsTest.java b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/FunctionsTest.java index c6939c31d92..2d33342fd2c 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/FunctionsTest.java +++ b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/FunctionsTest.java @@ -35,9 +35,6 @@ import org.junit.Test; * Test Ignite SQL functions. */ public class FunctionsTest extends AbstractBasicIntegrationTest { - /** */ - private static final Object[] NULL_RESULT = new Object[] { null }; - /** */ @Test public void testTimestampDiffWithFractionsOfSecond() { diff --git a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/LimitOffsetTest.java b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java similarity index 59% rename from modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/LimitOffsetTest.java rename to modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java index 266f2c7773f..aa9ba97e750 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/LimitOffsetTest.java +++ b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/LimitOffsetIntegrationTest.java @@ -15,7 +15,7 @@ * limitations under the License. */ -package org.apache.ignite.internal.processors.query.calcite; +package org.apache.ignite.internal.processors.query.calcite.integration; import java.math.BigDecimal; import java.util.Arrays; @@ -24,18 +24,13 @@ import org.apache.calcite.sql.validate.SqlValidatorException; import org.apache.ignite.IgniteCache; import org.apache.ignite.cache.CacheMode; import org.apache.ignite.cache.QueryEntity; -import org.apache.ignite.cache.query.FieldsQueryCursor; import org.apache.ignite.configuration.CacheConfiguration; import org.apache.ignite.configuration.IgniteConfiguration; -import org.apache.ignite.internal.IgniteEx; import org.apache.ignite.internal.processors.query.IgniteSQLException; -import org.apache.ignite.internal.processors.query.QueryEngine; import org.apache.ignite.internal.processors.query.calcite.exec.rel.AbstractNode; -import org.apache.ignite.internal.processors.query.calcite.util.Commons; 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; @@ -43,7 +38,7 @@ import static java.util.Collections.singletonList; /** * Limit / offset tests. */ -public class LimitOffsetTest extends GridCommonAbstractTest { +public class LimitOffsetIntegrationTest extends AbstractBasicIntegrationTest { /** */ private static IgniteCache<Integer, String> cacheRepl; @@ -51,11 +46,14 @@ public class LimitOffsetTest extends GridCommonAbstractTest { private static IgniteCache<Integer, String> cachePart; /** {@inheritDoc} */ - @Override protected void beforeTestsStarted() throws Exception { - startGrids(2); + @Override protected void beforeTest() throws Exception { + cacheRepl = client.cache("TEST_REPL"); + cachePart = client.cache("TEST_PART"); + } - cacheRepl = grid(0).cache("TEST_REPL"); - cachePart = grid(0).cache("TEST_PART"); + /** {@inheritDoc} */ + @Override protected void afterTest() throws Exception { + // Override method to keep caches after tests. } /** {@inheritDoc} */ @@ -76,7 +74,7 @@ public class LimitOffsetTest extends GridCommonAbstractTest { .setKeyFieldName("id") .setValueFieldName("val") .addQueryField("id", Integer.class.getName(), null) - .addQueryField("val", String.class.getName(), null);; + .addQueryField("val", String.class.getName(), null); return super.getConfiguration(igniteInstanceName) .setCacheConfiguration( @@ -93,117 +91,35 @@ public class LimitOffsetTest extends GridCommonAbstractTest { /** Tests correctness of fetch / offset params. */ @Test public void testInvalidLimitOffset() { - QueryEngine engine = engine(grid(0)); - String bigInt = BigDecimal.valueOf(10000000000L).toString(); - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL OFFSET " + bigInt + " ROWS"); - cursors.get(0).getAll(); - - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, "Illegal value of offset", SqlValidatorException.class)); - } - - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL FETCH FIRST " + bigInt + " ROWS ONLY"); - cursors.get(0).getAll(); - - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, "Illegal value of fetch / limit", SqlValidatorException.class)); - } - - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", "SELECT * FROM TEST_REPL LIMIT " + bigInt); - cursors.get(0).getAll(); + assertThrows("SELECT * FROM TEST_REPL OFFSET " + bigInt + " ROWS", + SqlValidatorException.class, "Illegal value of offset"); - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, "Illegal value of fetch / limit", SqlValidatorException.class)); - } + assertThrows("SELECT * FROM TEST_REPL FETCH FIRST " + bigInt + " ROWS ONLY", + SqlValidatorException.class, "Illegal value of fetch / limit"); - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL OFFSET -1 ROWS FETCH FIRST -1 ROWS ONLY"); - cursors.get(0).getAll(); + assertThrows("SELECT * FROM TEST_REPL LIMIT " + bigInt, + SqlValidatorException.class, "Illegal value of fetch / limit"); - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, IgniteSQLException.class)); - } + assertThrows("SELECT * FROM TEST_REPL OFFSET -1 ROWS FETCH FIRST -1 ROWS ONLY", + IgniteSQLException.class, null); - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL OFFSET -1 ROWS"); - cursors.get(0).getAll(); + assertThrows("SELECT * FROM TEST_REPL OFFSET -1 ROWS", + IgniteSQLException.class, null); - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, IgniteSQLException.class)); - } - - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL OFFSET 2+1 ROWS"); - cursors.get(0).getAll(); - - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, IgniteSQLException.class)); - } + assertThrows("SELECT * FROM TEST_REPL OFFSET 2+1 ROWS", + IgniteSQLException.class, null); // Check with parameters - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL OFFSET ? ROWS FETCH FIRST ? ROWS ONLY", -1, -1); - cursors.get(0).getAll(); + assertThrows("SELECT * FROM TEST_REPL OFFSET ? ROWS FETCH FIRST ? ROWS ONLY", + SqlValidatorException.class, "Illegal value of fetch / limit", -1, -1); - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, "Illegal value of fetch / limit", SqlValidatorException.class)); - } - - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL OFFSET ? ROWS", -1); - cursors.get(0).getAll(); - - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, "Illegal value of offset", SqlValidatorException.class)); - } + assertThrows("SELECT * FROM TEST_REPL OFFSET ? ROWS", + SqlValidatorException.class, "Illegal value of offset", -1); - try { - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", - "SELECT * FROM TEST_REPL FETCH FIRST ? ROWS ONLY", -1); - cursors.get(0).getAll(); - - fail(); - } - catch (Throwable e) { - assertTrue(e.toString(), X.hasCause(e, "Illegal value of fetch / limit", SqlValidatorException.class)); - } + assertThrows("SELECT * FROM TEST_REPL FETCH FIRST ? ROWS ONLY", + SqlValidatorException.class, "Illegal value of fetch / limit", -1); } /** @@ -239,13 +155,8 @@ public class LimitOffsetTest extends GridCommonAbstractTest { public void testLimitDistributed() throws Exception { fillCache(cachePart, 10_000); - QueryEngine engine = engine(grid(0)); - for (String order : F.asArray("id", "val")) { // Order by ID - without explicit IgniteSort node. - FieldsQueryCursor<List<?>> cur = engine.query(null, "PUBLIC", - "SELECT id FROM TEST_PART ORDER BY " + order + " LIMIT 1000 OFFSET 5000").get(0); - - List<List<?>> res = cur.getAll(); + List<List<?>> res = sql("SELECT id FROM TEST_PART ORDER BY " + order + " LIMIT 1000 OFFSET 5000"); assertEquals(1000, res.size()); @@ -254,11 +165,19 @@ public class LimitOffsetTest extends GridCommonAbstractTest { } } + /** */ + @Test + public void testOffsetOutOfRange() throws Exception { + fillCache(cachePart, 5); + + assertQuery("SELECT (SELECT id FROM TEST_PART ORDER BY id LIMIT 1 OFFSET 10)").returns(NULL_RESULT).check(); + } + /** * @param c Cache. * @param rows Rows count. */ - private void fillCache(IgniteCache c, int rows) throws InterruptedException { + private void fillCache(IgniteCache<Integer, String> c, int rows) throws InterruptedException { c.clear(); for (int i = 0; i < rows; ++i) @@ -277,8 +196,6 @@ public class LimitOffsetTest extends GridCommonAbstractTest { * @param sorted Use sorted query (adds ORDER BY). */ void checkQuery(int rows, int lim, int off, boolean param, boolean sorted) { - QueryEngine engine = engine(grid(0)); - String sql = createSql(lim, off, param, sorted); Object[] params; @@ -293,10 +210,7 @@ public class LimitOffsetTest extends GridCommonAbstractTest { log.info("SQL: " + sql + (param ? "params=" + Arrays.toString(params) : "")); - List<FieldsQueryCursor<List<?>>> cursors = - engine.query(null, "PUBLIC", sql, param ? params : X.EMPTY_OBJECT_ARRAY); - - List<List<?>> res = cursors.get(0).getAll(); + List<List<?>> res = sql(sql, params); assertEquals("Invalid results size. [rows=" + rows + ", limit=" + lim + ", off=" + off + ", res=" + res.size() + ']', expectedSize(rows, lim, off), res.size()); @@ -341,9 +255,4 @@ public class LimitOffsetTest extends GridCommonAbstractTest { return sb.toString(); } - - /** */ - private QueryEngine engine(IgniteEx grid) { - return Commons.lookupComponent(grid.context(), QueryEngine.class); - } } diff --git a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/LimitOffsetPlannerTest.java b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/LimitOffsetPlannerTest.java index 3e0f38aeb30..c773cb4f7ce 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/LimitOffsetPlannerTest.java +++ b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/LimitOffsetPlannerTest.java @@ -77,6 +77,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))); } /** */ diff --git a/modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java b/modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java index 44a6113e9bf..198d5743646 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java +++ b/modules/calcite/src/test/java/org/apache/ignite/testsuites/IntegrationTestSuite.java @@ -20,7 +20,6 @@ package org.apache.ignite.testsuites; import org.apache.ignite.internal.processors.query.calcite.CalciteQueryProcessorTest; import org.apache.ignite.internal.processors.query.calcite.CancelTest; import org.apache.ignite.internal.processors.query.calcite.DateTimeTest; -import org.apache.ignite.internal.processors.query.calcite.LimitOffsetTest; import org.apache.ignite.internal.processors.query.calcite.SqlFieldsQueryUsageTest; import org.apache.ignite.internal.processors.query.calcite.UnstableTopologyTest; import org.apache.ignite.internal.processors.query.calcite.integration.AggregatesIntegrationTest; @@ -39,6 +38,7 @@ import org.apache.ignite.internal.processors.query.calcite.integration.JoinInteg import org.apache.ignite.internal.processors.query.calcite.integration.KeepBinaryIntegrationTest; import org.apache.ignite.internal.processors.query.calcite.integration.KillCommandDdlIntegrationTest; import org.apache.ignite.internal.processors.query.calcite.integration.KillQueryCommandDdlIntegrationTest; +import org.apache.ignite.internal.processors.query.calcite.integration.LimitOffsetIntegrationTest; import org.apache.ignite.internal.processors.query.calcite.integration.LocalDateTimeSupportTest; import org.apache.ignite.internal.processors.query.calcite.integration.MemoryQuotasIntegrationTest; import org.apache.ignite.internal.processors.query.calcite.integration.MetadataIntegrationTest; @@ -78,7 +78,7 @@ import org.junit.runners.Suite; CalciteBasicSecondaryIndexIntegrationTest.class, CancelTest.class, DateTimeTest.class, - LimitOffsetTest.class, + LimitOffsetIntegrationTest.class, SqlFieldsQueryUsageTest.class, AggregatesIntegrationTest.class, MetadataIntegrationTest.class,