alex-plekhanov commented on code in PR #9987:
URL: https://github.com/apache/ignite/pull/9987#discussion_r854195980
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteSort.java:
##########
@@ -37,12 +37,35 @@
import
org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import static
org.apache.ignite.internal.processors.query.calcite.rel.IgniteLimit.FETCH_IS_PARAM_FACTOR;
+import static
org.apache.ignite.internal.processors.query.calcite.rel.IgniteLimit.OFFSET_IS_PARAM_FACTOR;
+import static
org.apache.ignite.internal.processors.query.calcite.rel.IgniteLimit.doubleFromRex;
import static
org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils.changeTraits;
/**
* Ignite sort operator.
*/
public class IgniteSort extends Sort implements IgniteRel {
+ /**
+ * Constructor.
+ *
+ * @param cluster Cluster.
+ * @param traits Trait set.
+ * @param child Input node.
+ * @param collation Collation.
+ * @param offset Offset.
+ * @param limit Limit.
+ */
+ public IgniteSort(
+ RelOptCluster cluster,
+ RelTraitSet traits,
+ RelNode child,
+ RelCollation collation,
+ RexNode offset,
+ RexNode limit) {
Review Comment:
Let's keep parameter name the sme as variable name (`fetch`)
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteSort.java:
##########
@@ -127,6 +155,11 @@ public IgniteSort(RelInput input) {
/** {@inheritDoc} */
@Override public IgniteRel clone(RelOptCluster cluster, List<IgniteRel>
inputs) {
- return new IgniteSort(cluster, getTraitSet(), sole(inputs), collation);
+ return new IgniteSort(cluster, getTraitSet(), sole(inputs), collation,
fetch, offset);
Review Comment:
Wrong fetch and offset order
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java:
##########
@@ -163,7 +163,7 @@ else if (!requiredCollation.satisfies(relCollation))
/**
* @return Integer value of the literal expression.
*/
- private double doubleFromRex(RexNode n, double def) {
+ static double doubleFromRex(RexNode n, double def) {
Review Comment:
Let's move it to `RexUtils` class. Also, javadoc and perhaps implementation
should be fixed (I don't understand why we convert it to Integer.class first).
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1049,6 +1049,27 @@ public void quantifiedCompTest() throws
InterruptedException {
.returns(13d)
.check();
+ assertQuery(client, "select salary from account where salary > SOME
(10, 11) ORDER BY salary OFFSET 1")
+ .returns(12d)
+ .returns(13d)
+ .returns(13d)
+ .returns(13d)
+ .check();
+
+ assertQuery(client, "select salary from account where salary > 11
ORDER BY salary LIMIT 3 OFFSET 1")
+ .returns(13d)
+ .returns(13d)
+ .returns(13d)
+ .check();
+
+ assertQuery(client, "select salary from account where salary > 11
ORDER BY salary LIMIT 0")
+ .returns()
Review Comment:
`QueryChecker` assume row per each `returns()`, here it looks like "returns
one row with no values". Perhaps it's better to add new method to
`QueryChecker`, like returnsEmptyResult or something like that.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessorTest.java:
##########
@@ -1049,6 +1049,27 @@ public void quantifiedCompTest() throws
InterruptedException {
.returns(13d)
.check();
+ assertQuery(client, "select salary from account where salary > SOME
(10, 11) ORDER BY salary OFFSET 1")
Review Comment:
Let's move limit and offset checks to `LimitOffsetTest` class (also move
`LimitOffsetTest` to `integration` package and inherit from
`AbstractBasicIntegrationTest`)
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/TraitUtils.java:
##########
@@ -123,16 +126,23 @@ else if (converter == RewindabilityTraitDef.INSTANCE)
}
/** */
- @Nullable public static RelNode convertCollation(RelOptPlanner planner,
- RelCollation toTrait, RelNode rel) {
+ @Nullable public static RelNode convertCollation(RelOptPlanner planner,
RelCollation toTrait, RelNode rel) {
RelCollation fromTrait = collation(rel);
if (fromTrait.satisfies(toTrait))
return rel;
RelTraitSet traits = rel.getTraitSet().replace(toTrait);
- return new IgniteSort(rel.getCluster(), traits, rel, toTrait);
+ Sort sortNode = null;
+
+ if (rel instanceof SortNode)
+ sortNode = (Sort)rel;
+ if (rel instanceof RelSubset && ((RelSubset)rel).getOriginal()
instanceof Sort)
Review Comment:
Looks fragile. Can we implement this logic in `SortConverterRule`?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -39,14 +41,33 @@
/** Rows buffer. */
private final PriorityQueue<Row> rows;
+ /** SQL select limit. Negative if disabled. */
+ private final int limit;
+
/**
* @param ctx Execution context.
* @param comp Rows comparator.
+ * @param offset Offset.
+ * @param limit Limit.
*/
- public SortNode(ExecutionContext<Row> ctx, RelDataType rowType,
Comparator<Row> comp) {
+ public SortNode(ExecutionContext<Row> ctx, RelDataType rowType,
Comparator<Row> comp,
+ @Nullable Supplier<Integer> offset, @Nullable Supplier<Integer> limit)
{
super(ctx, rowType);
rows = comp == null ? new PriorityQueue<>() : new
PriorityQueue<>(comp);
+
+ assert limit == null || limit.get() >= 0;
Review Comment:
Shouldn't be assert here, since limit and offset depends on user input, if
user set wrong values for limit and offset there should be correct exception
with correct error message.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java:
##########
@@ -41,10 +41,10 @@
/** */
public class IgniteLimit extends SingleRel implements IgniteRel {
/** In case the fetch value is a DYNAMIC_PARAM. */
- private static final double FETCH_IS_PARAM_FACTOR = 0.01;
+ static final double FETCH_IS_PARAM_FACTOR = 0.01;
Review Comment:
Let's move it to `IgniteCost` class
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteSort.java:
##########
@@ -109,14 +130,21 @@ public IgniteSort(RelInput input) {
/** {@inheritDoc} */
@Override public RelOptCost computeSelfCost(RelOptPlanner planner,
RelMetadataQuery mq) {
- double rows = mq.getRowCount(getInput());
+ double inputRows = mq.getRowCount(getInput());
- double cpuCost = rows * IgniteCost.ROW_PASS_THROUGH_COST +
Util.nLogN(rows) * IgniteCost.ROW_COMPARISON_COST;
- double memory = rows * getRowType().getFieldCount() *
IgniteCost.AVERAGE_FIELD_SIZE;
+ double lim = fetch != null ? doubleFromRex(fetch, inputRows *
FETCH_IS_PARAM_FACTOR) : inputRows;
Review Comment:
`lim` -> `fetch`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java:
##########
@@ -463,7 +463,11 @@ public LogicalRelImplementor(
@Override public Node<Row> visit(IgniteSort rel) {
RelCollation collation = rel.getCollation();
- SortNode<Row> node = new SortNode<>(ctx, rel.getRowType(),
expressionFactory.comparator(collation));
+ Supplier<Integer> limit = (rel.fetch == null) ? null :
expressionFactory.execute(rel.fetch);
Review Comment:
Let's use the same variable name for `fetch`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/SortNode.java:
##########
@@ -91,6 +112,12 @@ else if (!inLoop)
rows.add(row);
+ if (limit >= 0 && rows.size() > limit) {
+ AtomicInteger i = new AtomicInteger();
+
+ rows.removeIf(r -> i.incrementAndGet() == rows.size());
Review Comment:
Such approach makes the whole ticket useless, since it's O(N) complexity for
insertion instead of O(Log(N)).
Let's use `GridBoundedPriorityQueue`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteLimit.java:
##########
@@ -41,10 +41,10 @@
/** */
public class IgniteLimit extends SingleRel implements IgniteRel {
/** In case the fetch value is a DYNAMIC_PARAM. */
- private static final double FETCH_IS_PARAM_FACTOR = 0.01;
+ static final double FETCH_IS_PARAM_FACTOR = 0.01;
/** In case the offset value is a DYNAMIC_PARAM. */
- private static final double OFFSET_IS_PARAM_FACTOR = 0.5;
+ static final double OFFSET_IS_PARAM_FACTOR = 0.5;
Review Comment:
Let's move it to `IgniteCost` class
--
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]
