This is an automated email from the ASF dual-hosted git repository.
korlov 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 6a7a6e08cd2 IGNITE-25530 Sql. Explain. Confusing printout of Sort node
(#6044)
6a7a6e08cd2 is described below
commit 6a7a6e08cd238bef5a9f7967c3005da53aa168c0
Author: korlov42 <[email protected]>
AuthorDate: Tue Jun 17 11:35:08 2025 +0300
IGNITE-25530 Sql. Explain. Confusing printout of Sort node (#6044)
---
.../sql/group1/explain/sort_and_limit.test | 3 +-
.../internal/sql/engine/exec/ExecutionContext.java | 5 +--
.../sql/engine/exec/exp/RexToLixTranslator.java | 6 +--
.../internal/sql/engine/exec/rel/LimitNode.java | 2 +-
.../internal/sql/engine/exec/rel/SortNode.java | 28 +++++++++---
.../internal/sql/engine/rel/IgniteLimit.java | 11 ++++-
.../ignite/internal/sql/engine/rel/IgniteSort.java | 2 +-
.../sql/engine/rule/SortConverterRule.java | 52 +++++++++++++++++++++-
.../internal/sql/engine/util/IgniteMethod.java | 4 --
.../sql/engine/exec/rel/LimitExecutionTest.java | 16 +++++--
.../sql/engine/planner/LimitOffsetPlannerTest.java | 12 ++---
11 files changed, 106 insertions(+), 35 deletions(-)
diff --git
a/modules/sql-engine/src/integrationTest/sql/group1/explain/sort_and_limit.test
b/modules/sql-engine/src/integrationTest/sql/group1/explain/sort_and_limit.test
index c0c7e6fdadd..98ec081cb7d 100644
---
a/modules/sql-engine/src/integrationTest/sql/group1/explain/sort_and_limit.test
+++
b/modules/sql-engine/src/integrationTest/sql/group1/explain/sort_and_limit.test
@@ -118,8 +118,7 @@ Limit
est: (rows=30)
Sort
collation: [C1 ASC, C2 ASC]
- offset: 5
- fetch: 30
+ fetch: 35
est: (rows=35)
Project
fieldNames: [C1, C2]
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionContext.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionContext.java
index fcfb3e52c7c..76454b00d8d 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionContext.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionContext.java
@@ -20,7 +20,6 @@ package org.apache.ignite.internal.sql.engine.exec;
import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
-import java.lang.reflect.Type;
import java.time.Clock;
import java.time.Instant;
import java.time.ZoneId;
@@ -301,14 +300,14 @@ public class ExecutionContext<RowT> implements
DataContext {
}
if (name.startsWith("?")) {
- return getParameter(name, null);
+ return getParameter(name);
} else {
return params.get(name);
}
}
/** Gets dynamic parameters by name. */
- public @Nullable Object getParameter(String name, @Nullable Type
storageType) {
+ private @Nullable Object getParameter(String name) {
assert name.startsWith("?") : name;
Object param = params.get(name);
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexToLixTranslator.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexToLixTranslator.java
index 5e7faeba382..e656eec2df8 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexToLixTranslator.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/RexToLixTranslator.java
@@ -93,7 +93,6 @@ import org.apache.calcite.sql.validate.SqlConformance;
import org.apache.calcite.util.BuiltInMethod;
import org.apache.calcite.util.ControlFlowException;
import org.apache.calcite.util.Pair;
-import org.apache.ignite.internal.sql.engine.type.IgniteTypeFactory;
import org.apache.ignite.internal.sql.engine.util.IgniteMethod;
import org.apache.ignite.internal.sql.engine.util.Primitives;
import org.checkerframework.checker.nullness.qual.Nullable;
@@ -1555,9 +1554,8 @@ public class RexToLixTranslator implements
RexVisitor<RexToLixTranslator.Result>
Expressions.call(root, BuiltInMethod.DATA_CONTEXT_GET.method,
Expressions.constant("?" + dynamicParam.getIndex())),
storageType);*/
- final Type paramType = ((IgniteTypeFactory)
typeFactory).getResultClass(dynamicParam.getType());
- final Expression ctxGet = Expressions.call(root,
IgniteMethod.CONTEXT_GET_PARAMETER_VALUE.method(),
- Expressions.constant("?" + dynamicParam.getIndex()),
Expressions.constant(paramType));
+ final Expression ctxGet = Expressions.call(root,
BuiltInMethod.DATA_CONTEXT_GET.method,
+ Expressions.constant("?" + dynamicParam.getIndex()));
final Expression valueExpression = ConverterUtils.convert(ctxGet,
dynamicParam.getType());
final ParameterExpression valueVariable =
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 e0069f46ca5..48f00e9026b 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
@@ -34,7 +34,7 @@ public class LimitNode<RowT> extends AbstractNode<RowT>
implements SingleNode<Ro
/** Fetch can be unset. */
private final boolean fetchUndefined;
- /** Already processed (pushed to upstream) rows count. */
+ /** Already processed (pushed to downstream) rows count. */
private long rowsProcessed;
/** Waiting results counter. */
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 07a66d3fa3e..bb93648eb87 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
@@ -44,7 +44,9 @@ public class SortNode<RowT> extends AbstractNode<RowT>
implements SingleNode<Row
private final PriorityQueue<RowT> rows;
/** SQL select limit. Negative if disabled. */
- private final long limit;
+ private final long fetch;
+
+ private final long offset;
/** Reverse-ordered rows in case of limited sort. */
private List<RowT> reversed;
@@ -60,13 +62,24 @@ public class SortNode<RowT> extends AbstractNode<RowT>
implements SingleNode<Row
public SortNode(ExecutionContext<RowT> ctx,
Comparator<RowT> comp,
long offset,
- long fetch) {
+ long fetch
+ ) {
super(ctx);
assert fetch == -1 || fetch >= 0;
assert offset >= 0;
- limit = fetch == -1 ? -1 : IgniteMath.addExact(fetch, offset);
+ // Offset must be set only together with fetch.
+ // This is limitation is current implementation, and SortConverterRule
+ // should not produce unsupported variant.
+ if (offset > 0 && fetch == -1) {
+ throw new AssertionError("Offset-only case is not supported by
Sort node");
+ }
+
+ this.fetch = fetch;
+ this.offset = offset;
+
+ long limit = fetch == -1 ? -1 : IgniteMath.addExact(fetch, offset);
if (limit < 1 || limit > Integer.MAX_VALUE) {
rows = new PriorityQueue<>(comp);
@@ -154,7 +167,8 @@ public class SortNode<RowT> extends AbstractNode<RowT>
implements SingleNode<Row
buf.app("class=").app(getClass().getSimpleName())
.app(", requested=").app(requested)
.app(", waiting=").app(waiting)
- .app(", limit=").app(limit);
+ .app(", fetch=").app(fetch)
+ .app(", offset=").app(offset);
}
private void flush() throws Exception {
@@ -165,12 +179,12 @@ public class SortNode<RowT> extends AbstractNode<RowT>
implements SingleNode<Row
inLoop = true;
try {
// Prepare final order (reversed).
- if (limit > 0 && !rows.isEmpty()) {
+ if (fetch > 0 && !rows.isEmpty()) {
if (reversed == null) {
reversed = new ArrayList<>(rows.size());
}
- while (!rows.isEmpty()) {
+ while (rows.size() > offset) {
reversed.add(rows.poll());
if (++processed >= inBufSize) {
@@ -181,6 +195,8 @@ public class SortNode<RowT> extends AbstractNode<RowT>
implements SingleNode<Row
}
}
+ rows.clear();
+
processed = 0;
}
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 55283d9df8b..27c0e17e053 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
@@ -176,12 +176,19 @@ public class IgniteLimit extends SingleRel implements
IgniteRel {
/** {@inheritDoc} */
@Override
public double estimateRowCount(RelMetadataQuery mq) {
- double inputRowCount = mq.getRowCount(getInput());
+ return estimateRowCount(mq.getRowCount(getInput()), offset, fetch);
+ }
+ /** Returns the estimated row count based on provided input and offset and
fetch attributes. */
+ public static double estimateRowCount(
+ double inputRowCount,
+ @Nullable RexNode offset,
+ @Nullable RexNode fetch
+ ) {
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.max(0, Math.min(lim, inputRowCount - off));
+ return Math.max(1, Math.min(lim, inputRowCount - off));
}
/**
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteSort.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteSort.java
index 96caf9227af..6cdbdfb1413 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteSort.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/IgniteSort.java
@@ -151,7 +151,7 @@ public class IgniteSort extends Sort implements IgniteRel {
/** {@inheritDoc} */
@Override
public double estimateRowCount(RelMetadataQuery mq) {
- return memRows(mq.getRowCount(getInput()));
+ return IgniteLimit.estimateRowCount(mq.getRowCount(getInput()),
offset, fetch);
}
/** {@inheritDoc} */
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/SortConverterRule.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/SortConverterRule.java
index 37644d75351..21aa941db7c 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/SortConverterRule.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/SortConverterRule.java
@@ -17,6 +17,8 @@
package org.apache.ignite.internal.sql.engine.rule;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Map;
import org.apache.calcite.plan.RelOptCluster;
import org.apache.calcite.plan.RelOptRule;
@@ -27,11 +29,19 @@ import org.apache.calcite.rel.RelCollations;
import org.apache.calcite.rel.RelNode;
import org.apache.calcite.rel.core.Sort;
import org.apache.calcite.rel.logical.LogicalSort;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexExecutor;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexUtil;
import org.apache.ignite.internal.sql.engine.rel.IgniteConvention;
import org.apache.ignite.internal.sql.engine.rel.IgniteLimit;
import org.apache.ignite.internal.sql.engine.rel.IgniteSort;
+import org.apache.ignite.internal.sql.engine.sql.fun.IgniteSqlOperatorTable;
import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.util.ExceptionUtils;
+import org.apache.ignite.internal.util.IgniteUtils;
import org.immutables.value.Value;
+import org.jetbrains.annotations.Nullable;
/**
* Converter rule for sort operator.
@@ -79,8 +89,8 @@ public class SortConverterRule extends
RelRule<SortConverterRule.Config> {
cluster.traitSetOf(IgniteConvention.INSTANCE).replace(sort.getCollation()),
convert(sort.getInput(),
cluster.traitSetOf(IgniteConvention.INSTANCE)),
sort.getCollation(),
- sort.offset,
- sort.fetch
+ null,
+ createLimitForSort(cluster.getPlanner().getExecutor(),
cluster.getRexBuilder(), sort.offset, sort.fetch)
);
call.transformTo(
@@ -99,4 +109,42 @@ public class SortConverterRule extends
RelRule<SortConverterRule.Config> {
call.transformTo(new IgniteSort(cluster, outTraits, input,
sort.getCollation()));
}
}
+
+ private static @Nullable RexNode createLimitForSort(
+ @Nullable RexExecutor executor, RexBuilder builder, @Nullable
RexNode offset, @Nullable RexNode fetch
+ ) {
+ if (fetch == null) {
+ // Current implementation of SortNode cannot handle offset-only
case.
+ return null;
+ }
+
+ if (offset != null) {
+ boolean shouldTryToSimplify = RexUtil.isLiteral(fetch, false)
+ && RexUtil.isLiteral(offset, false);
+
+ fetch = builder.makeCall(IgniteSqlOperatorTable.PLUS, fetch,
offset);
+
+ if (shouldTryToSimplify && executor != null) {
+ try {
+ List<RexNode> result = new ArrayList<>();
+ executor.reduce(builder, List.of(fetch), result);
+
+ assert result.size() <= 1 : result;
+
+ if (result.size() == 1) {
+ fetch = result.get(0);
+ }
+ } catch (Exception ex) {
+ if (IgniteUtils.assertionsEnabled()) {
+ ExceptionUtils.sneakyThrow(ex);
+ }
+
+ // Just ignore the exception, we will deal with this
expression later again,
+ // and next time we might have all the required context to
evaluate it.
+ }
+ }
+ }
+
+ return fetch;
+ }
}
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteMethod.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteMethod.java
index b2b92e1d458..02483328fb7 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteMethod.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteMethod.java
@@ -20,7 +20,6 @@ package org.apache.ignite.internal.sql.engine.util;
import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
import java.lang.reflect.Method;
-import java.lang.reflect.Type;
import java.math.BigDecimal;
import java.util.Arrays;
import java.util.Objects;
@@ -53,9 +52,6 @@ public enum IgniteMethod {
/** See {@link ExecutionContext#correlatedVariable(int)}. */
CONTEXT_GET_CORRELATED_VALUE(ExecutionContext.class, "correlatedVariable",
int.class),
- /** See {@link ExecutionContext#getParameter(String, Type)}. */
- CONTEXT_GET_PARAMETER_VALUE(ExecutionContext.class, "getParameter",
String.class, Type.class),
-
/** See {@link IgniteSqlDateTimeUtils#subtractTimeZoneOffset(long,
TimeZone)}. **/
SUBTRACT_TIMEZONE_OFFSET(IgniteSqlDateTimeUtils.class,
"subtractTimeZoneOffset", long.class, TimeZone.class),
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 a085c3abb43..035878365a6 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
@@ -18,6 +18,7 @@
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.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -33,7 +34,17 @@ import org.junit.jupiter.api.Test;
/**
* Test LimitNode execution.
*/
+@SuppressWarnings({"ThrowableNotThrown", "ResultOfObjectAllocationIgnored",
"resource"})
public class LimitExecutionTest extends AbstractExecutionTest<Object[]> {
+ @Test
+ void offsetOnlyCaseIsNotSupportedBySortNode() {
+ assertThrowsWithCause(
+ () -> new SortNode<>(executionContext(),
LimitExecutionTest::compareArrays, 10, -1),
+ AssertionError.class,
+ "Offset-only case is not supported by Sort node"
+ );
+ }
+
/** Tests correct results fetched with Limit node. */
@Test
public void testLimit() {
@@ -61,13 +72,10 @@ public class LimitExecutionTest extends
AbstractExecutionTest<Object[]> {
int bufSize = IN_BUFFER_SIZE;
checkLimitSort(0, 1);
- checkLimitSort(1, 0);
checkLimitSort(1, 1);
checkLimitSort(0, bufSize);
- checkLimitSort(bufSize, 0);
checkLimitSort(bufSize, bufSize);
checkLimitSort(bufSize - 1, 1);
- checkLimitSort(2000, 0);
checkLimitSort(0, 3000);
checkLimitSort(2000, 3000);
}
@@ -99,7 +107,7 @@ public class LimitExecutionTest extends
AbstractExecutionTest<Object[]> {
sortNode.register(srcNode);
- for (int i = 0; i < offset + fetch; i++) {
+ for (int i = offset; i < offset + fetch; i++) {
assertTrue(rootNode.hasNext());
assertEquals(i, rootNode.next()[0]);
}
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 e844676cbf9..202a53f7808 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
@@ -88,8 +88,8 @@ public class LimitOffsetPlannerTest extends
AbstractPlannerTest {
isInstanceOf(IgniteLimit.class)
.and(input(isInstanceOf(IgniteExchange.class)
.and(input(isInstanceOf(IgniteSort.class)
- .and(s -> doubleFromRex(s.fetch, -1)
== 5.0)
- .and(s -> doubleFromRex(s.offset, -1)
== 10.0))))));
+ .and(s -> doubleFromRex(s.fetch, -1)
== 15.0)
+ .and(s -> s.offset == null))))));
// Same simple case but witout offset.
assertPlan("SELECT * FROM TEST ORDER BY ID LIMIT 5", publicSchema,
@@ -163,8 +163,8 @@ public class LimitOffsetPlannerTest extends
AbstractPlannerTest {
.and(input(isInstanceOf(IgniteExchange.class)
.and(e -> e.distribution() ==
IgniteDistributions.single())
.and(input(isInstanceOf(IgniteSort.class)
- .and(s -> doubleFromRex(s.offset, -1)
== 1)
- .and(s -> doubleFromRex(s.fetch, -1)
== 1)))))));
+ .and(s -> doubleFromRex(s.fetch, -1)
== 2)
+ .and(s -> s.offset == null)))))));
publicSchema = createSchemaWithTable(IgniteDistributions.random(),
"ID");
@@ -262,8 +262,8 @@ public class LimitOffsetPlannerTest extends
AbstractPlannerTest {
.and(input(isInstanceOf(IgniteExchange.class)
.and(input(
isInstanceOf(IgniteSort.class)
- .and(l ->
doubleFromRex(l.offset, -1) == 2.0)
- .and(l ->
doubleFromRex(l.fetch, -1) == 3.0)
+ .and(l -> l.offset ==
null)
+ .and(l ->
doubleFromRex(l.fetch, -1) == 5.0)
))
))
))
