You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/09/26 07:04:45 UTC
[GitHub] [ignite] korlov42 commented on a diff in pull request #10251: IGNITE-15425 SQL Calcite: Add native support for SEARCH/SARG operator
korlov42 commented on code in PR #10251:
URL: https://github.com/apache/ignite/pull/10251#discussion_r977683560
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/RexUtils.java:
##########
@@ -368,39 +491,64 @@ private static Map<Integer, List<RexCall>> mapPredicatesToFields(RexNode conditi
}
/** */
- private static RexNode extractRef(RexCall call) {
+ private static RexNode extractRefFromBinary(RexCall call, RelOptCluster cluster) {
assert isBinaryComparison(call);
RexNode leftOp = call.getOperands().get(0);
RexNode rightOp = call.getOperands().get(1);
- leftOp = RexUtil.removeCast(leftOp);
- rightOp = RexUtil.removeCast(rightOp);
+ leftOp = removeCast(leftOp);
+ rightOp = removeCast(rightOp);
- if ((leftOp instanceof RexLocalRef || leftOp instanceof RexInputRef) && idxOpSupports(rightOp))
+ // Can proceed without ref cast only if cast was redundant in terms of values comparison.
+ if ((leftOp instanceof RexLocalRef || leftOp instanceof RexInputRef) && idxOpSupports(rightOp) &&
+ !TypeUtils.needCast(cluster.getTypeFactory(), leftOp.getType(), call.getOperands().get(0).getType()))
return leftOp;
- else if ((rightOp instanceof RexLocalRef || rightOp instanceof RexInputRef) && idxOpSupports(leftOp))
+ else if ((rightOp instanceof RexLocalRef || rightOp instanceof RexInputRef) && idxOpSupports(leftOp) &&
+ !TypeUtils.needCast(cluster.getTypeFactory(), rightOp.getType(), call.getOperands().get(1).getType()))
return rightOp;
return null;
}
+ /** */
+ private static RexNode extractRefFromFirstOperand(RexCall call, RelOptCluster cluster) {
+ assert isSupportedTreeComparison(call);
+
+ RexNode op = call.getOperands().get(0);
+
+ op = removeCast(op);
+
+ // Can proceed without ref cast only if cast was redundant in terms of values comparison.
+ if ((op instanceof RexLocalRef || op instanceof RexInputRef) &&
Review Comment:
```suggestion
if ((op instanceof RexSlot) &&
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/AbstractIndexScan.java:
##########
@@ -58,33 +55,36 @@
* @param ectx Execution context.
* @param idx Physical index.
* @param filters Additional filters.
- * @param lowerBound Lower index scan bound.
- * @param upperBound Upper index scan bound.
+ * @param boundsValues Index scan bounds.
*/
protected AbstractIndexScan(
ExecutionContext<Row> ectx,
RelDataType rowType,
TreeIndex<IdxRow> idx,
Predicate<Row> filters,
- Supplier<Row> lowerBound,
- Supplier<Row> upperBound,
+ Iterable<BoundsValues<Row>> boundsValues,
Function<Row, Row> rowTransformer
) {
this.ectx = ectx;
this.rowType = rowType;
this.idx = idx;
this.filters = filters;
- this.lowerBound = lowerBound;
- this.upperBound = upperBound;
+ this.boundsValues = boundsValues;
this.rowTransformer = rowTransformer;
}
/** {@inheritDoc} */
@Override public synchronized Iterator<Row> iterator() {
- IdxRow lower = lowerBound == null ? null : row2indexRow(lowerBound.get());
- IdxRow upper = upperBound == null ? null : row2indexRow(upperBound.get());
+ if (boundsValues == null)
+ return new IteratorImpl(idx.find(null, null, true, true, indexQueryContext()));
+
+ return F.flat(F.iterator(boundsValues, bounds -> {
+ IdxRow lower = bounds.lower() == null ? null : row2indexRow(bounds.lower());
+ IdxRow upper = bounds.upper() == null ? null : row2indexRow(bounds.upper());
- return new IteratorImpl(idx.find(lower, upper, indexQueryContext()));
Review Comment:
does it make sense to introduce custom iterator with only one lookup ahead (under separate ticket, probably)?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ExpressionFactoryImpl.java:
##########
@@ -270,6 +279,136 @@ private static int compare(Object o1, Object o2, int nullComparison) {
return rows;
}
+ /** {@inheritDoc} */
+ @Override public Iterable<BoundsValues<Row>> boundsValues(
+ List<SearchBounds> searchBounds,
+ RelCollation collation,
+ RelDataType rowType
+ ) {
+ RowFactory<Row> rowFactory = ctx.rowHandler().factory(typeFactory, rowType);
+
+ List<BoundsValues<Row>> boundsValues = new ArrayList<>();
+
+ expandBounds(
+ boundsValues,
+ searchBounds,
+ rowType,
+ rowFactory,
+ collation.getKeys(),
+ 0,
+ Arrays.asList(new RexNode[searchBounds.size()]),
+ Arrays.asList(new RexNode[searchBounds.size()]),
+ true,
+ true
+ );
+
+ Comparator<Row> comparator = comparator(collation);
+
+ return new Iterable<BoundsValues<Row>>() {
+ @NotNull @Override public Iterator<BoundsValues<Row>> iterator() {
+ boundsValues.forEach(b -> ((BoundsValuesImpl)b).clearCache());
+
+ if (boundsValues.size() == 1)
+ return boundsValues.iterator();
+
+ // Sort bound values using collation comparator to produce sorted output. There should be no ranges
+ // intersection between bounds.
+ boundsValues.sort((o1, o2) -> comparator.compare(o1.lower(), o2.lower()));
Review Comment:
Why do we sort bounds every time?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ExpressionFactoryImpl.java:
##########
@@ -270,6 +279,136 @@ private static int compare(Object o1, Object o2, int nullComparison) {
return rows;
}
+ /** {@inheritDoc} */
+ @Override public Iterable<BoundsValues<Row>> boundsValues(
+ List<SearchBounds> searchBounds,
+ RelCollation collation,
+ RelDataType rowType
+ ) {
+ RowFactory<Row> rowFactory = ctx.rowHandler().factory(typeFactory, rowType);
+
+ List<BoundsValues<Row>> boundsValues = new ArrayList<>();
+
+ expandBounds(
+ boundsValues,
+ searchBounds,
+ rowType,
+ rowFactory,
+ collation.getKeys(),
+ 0,
+ Arrays.asList(new RexNode[searchBounds.size()]),
+ Arrays.asList(new RexNode[searchBounds.size()]),
+ true,
+ true
+ );
+
+ Comparator<Row> comparator = comparator(collation);
+
+ return new Iterable<BoundsValues<Row>>() {
+ @NotNull @Override public Iterator<BoundsValues<Row>> iterator() {
+ boundsValues.forEach(b -> ((BoundsValuesImpl)b).clearCache());
+
+ if (boundsValues.size() == 1)
+ return boundsValues.iterator();
+
+ // Sort bound values using collation comparator to produce sorted output. There should be no ranges
+ // intersection between bounds.
+ boundsValues.sort((o1, o2) -> comparator.compare(o1.lower(), o2.lower()));
+
+ return boundsValues.iterator();
+ }
+ };
+ }
+
+ /**
+ * Expand search bounds to list of tuples (lower row/upper row).
Review Comment:
```suggestion
* Converts column-oriented {@link SearchBounds} to a row-oriented {@link BoundsValues}.
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/bounds/SearchBounds.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.processors.query.calcite.prepare.bounds;
+
+import org.apache.calcite.rex.RexNode;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Bounds holder for search row.
+ */
+public abstract class SearchBounds {
Review Comment:
Let's add to javadoc that this class is describing bounds on per-column basis, and such choice was made to optimise serialisation of a plan
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/RexUtils.java:
##########
@@ -368,39 +491,64 @@ private static Map<Integer, List<RexCall>> mapPredicatesToFields(RexNode conditi
}
/** */
- private static RexNode extractRef(RexCall call) {
+ private static RexNode extractRefFromBinary(RexCall call, RelOptCluster cluster) {
assert isBinaryComparison(call);
RexNode leftOp = call.getOperands().get(0);
RexNode rightOp = call.getOperands().get(1);
- leftOp = RexUtil.removeCast(leftOp);
- rightOp = RexUtil.removeCast(rightOp);
+ leftOp = removeCast(leftOp);
+ rightOp = removeCast(rightOp);
- if ((leftOp instanceof RexLocalRef || leftOp instanceof RexInputRef) && idxOpSupports(rightOp))
+ // Can proceed without ref cast only if cast was redundant in terms of values comparison.
+ if ((leftOp instanceof RexLocalRef || leftOp instanceof RexInputRef) && idxOpSupports(rightOp) &&
+ !TypeUtils.needCast(cluster.getTypeFactory(), leftOp.getType(), call.getOperands().get(0).getType()))
return leftOp;
- else if ((rightOp instanceof RexLocalRef || rightOp instanceof RexInputRef) && idxOpSupports(leftOp))
+ else if ((rightOp instanceof RexLocalRef || rightOp instanceof RexInputRef) && idxOpSupports(leftOp) &&
+ !TypeUtils.needCast(cluster.getTypeFactory(), rightOp.getType(), call.getOperands().get(1).getType()))
return rightOp;
return null;
}
+ /** */
+ private static RexNode extractRefFromFirstOperand(RexCall call, RelOptCluster cluster) {
Review Comment:
let's rework this in a way to extract reference from the N-th operand. This method could be reused in `extractRefFromBinary ` as well
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/BoundsValues.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.processors.query.calcite.exec.exp;
+
+/** Search rows values. */
+public interface BoundsValues<Row> {
Review Comment:
Probably, it would be better to rename this to `RangeCondition`.
Also, I would change the javadoc to something like this:
```
A range condition is a search condition which represents a comparison predicate or a BETWEEN predicate.
<p>Used to define bounds of a range scan.
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org