mihaibudiu commented on code in PR #5385:
URL: https://github.com/apache/calcite/pull/5385#discussion_r4518658506
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -287,6 +302,128 @@ public static List<CorrelationId>
getVariablesSetAndUsed(RelNode rel0,
return result;
}
+ /** Returns whether a relational expression, its inputs and sub-queries are
+ * deterministic according to their operator declarations.
+ *
+ * <p>Assumes parameters, correlation bindings, query context and table
contents
+ * stay fixed within the query. Function calls use
Review Comment:
what does it mean "they stay fixed"?
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -4570,6 +4707,33 @@ private static RelNode resetHints(Hintable hintable) {
}
}
+ /** Checks operator declarations and expression operands for determinism
Review Comment:
Does it throw when it finds a non-deterministic operator?
That needs to be part of the javadoc
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -287,6 +302,128 @@ public static List<CorrelationId>
getVariablesSetAndUsed(RelNode rel0,
return result;
}
+ /** Returns whether a relational expression, its inputs and sub-queries are
+ * deterministic according to their operator declarations.
+ *
+ * <p>Assumes parameters, correlation bindings, query context and table
contents
+ * stay fixed within the query. Function calls use
+ * {@link SqlOperator#isDeterministic()}, which allows query-stable functions
+ * such as {@code CURRENT_TIMESTAMP}. A {@link Sample} must declare
repeatability.
+ * Unknown relational operators return false. Planner wrappers are not
expanded.
+ *
+ * <p>Input order must also be stable where it affects the result. This
method
+ * does not establish that property, for example for an unordered FETCH.
+ * It does not trim fields or modify the expression.
+ */
+ public static boolean isDeterministic(RelNode rel) {
+ if (rel instanceof Sample
+ && !((Sample) rel).getSamplingParameters().isRepeatable()) {
+ return false;
+ }
+ if (rel instanceof Aggregate
+ || rel instanceof Window
+ || rel instanceof Match
+ || rel instanceof Project
+ || rel instanceof Filter
+ || rel instanceof Calc
+ || rel instanceof Join
+ || rel instanceof Correlate
+ || rel instanceof Sort
+ || rel instanceof SetOp
+ || rel instanceof Exchange
+ || rel instanceof TableScan
+ || rel instanceof Values
+ || rel instanceof TableFunctionScan
+ || rel instanceof Snapshot
+ || rel instanceof Collect
+ || rel instanceof Uncollect
+ || rel instanceof Sample) {
+ try {
+ if (!tryVisitExpressions(rel, DeterminismChecker.INSTANCE)) {
+ return false;
+ }
+ if (rel instanceof Aggregate) {
+ for (AggregateCall call : ((Aggregate) rel).getAggCallList()) {
+ if (!call.getAggregation().isDeterministic()) {
+ return false;
+ }
+ }
+ }
+ for (RelNode input : rel.getInputs()) {
+ if (!isDeterministic(input)) {
+ return false;
+ }
+ }
+ return true;
+ } catch (Util.FoundOne e) {
+ Util.swallow(e, null);
+ }
+ }
+ return false;
+ }
+
+ /** Visits a supported node's expression fields for read-only analysis.
Review Comment:
applies a Shuttle to the expression fields of a Rel node (but not to its
inputs)?
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -11242,3 +11242,78 @@ order by e.ename;
(14 rows)
!ok
+
+
+!if (use_old_decorr) {
+# Test case for [CALCITE-7914] Correlation keys must use the values already
Review Comment:
please describe how the results were validated.
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -287,6 +302,128 @@ public static List<CorrelationId>
getVariablesSetAndUsed(RelNode rel0,
return result;
}
+ /** Returns whether a relational expression, its inputs and sub-queries are
+ * deterministic according to their operator declarations.
+ *
+ * <p>Assumes parameters, correlation bindings, query context and table
contents
+ * stay fixed within the query. Function calls use
+ * {@link SqlOperator#isDeterministic()}, which allows query-stable functions
+ * such as {@code CURRENT_TIMESTAMP}. A {@link Sample} must declare
repeatability.
+ * Unknown relational operators return false. Planner wrappers are not
expanded.
+ *
+ * <p>Input order must also be stable where it affects the result. This
method
+ * does not establish that property, for example for an unordered FETCH.
+ * It does not trim fields or modify the expression.
Review Comment:
why would it modify the expression? It is supposed to be a read-only
analysis.
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -11242,3 +11242,78 @@ order by e.ename;
(14 rows)
!ok
+
+
+!if (use_old_decorr) {
+# Test case for [CALCITE-7914] Correlation keys must use the values already
Review Comment:
Typically [CALCITE-7914] is always followed by the issue title.
The description of the test can come next.
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -287,6 +302,128 @@ public static List<CorrelationId>
getVariablesSetAndUsed(RelNode rel0,
return result;
}
+ /** Returns whether a relational expression, its inputs and sub-queries are
+ * deterministic according to their operator declarations.
Review Comment:
well, you need to describe what non-deterministic means, or `@link` to its
definition.
We agreed there are multiple possible definitions.
##########
core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java:
##########
@@ -107,17 +108,16 @@ private static boolean isDirectFieldAccess(RexNode node,
CorrelationId id) {
RelNode left = correlate.getLeft().accept(this);
RelNode right = correlate.getRight().accept(this);
int oldLeft = left.getRowType().getFieldCount();
- // Find the correlated expressions from the right side that can be moved
to the left
- Set<RexNode> callsWithCorrelationInRight =
- findCorrelationDependentCalls(correlate.getCorrelationId(), right);
- // Only direct field accesses on the correlation variable, such as
- // $cor0.DEPTNO, are left in place. A nested field access, such as
- // $cor0.REC.DEPTNO, is extracted
- boolean isTrivialCorrelation =
- callsWithCorrelationInRight.stream()
- .allMatch(exp -> isDirectFieldAccess(exp,
correlate.getCorrelationId()));
- // Early exit condition
- if (isTrivialCorrelation) {
+ final Map<RexNode, Integer> expressions = new LinkedHashMap<>();
Review Comment:
the old version had some useful comments, can there be some for the new
version too?
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -288,6 +288,18 @@ public static RelNode decorrelateQuery(RelNode rootRel,
return newRootRel;
}
+ /** Trims the right input, preserving its output fields and the left input.
+ * Submits an equivalent Correlate if the plan changes and the trimmed right
Review Comment:
where does it submit the correlate?
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1035,7 +1071,14 @@ private static void shiftMapping(Map<Integer, Integer>
mapping, int startIndex,
*/
public @Nullable Frame getInvoke(RelNode r, boolean isCorVarDefined,
@Nullable RelNode parent, boolean parentPropagatesNullValues) {
- final Frame frame = dispatcher.invoke(r, isCorVarDefined,
parentPropagatesNullValues);
+ final Frame frame;
+ if (!(r instanceof Correlate) && !r.getVariablesSet().isEmpty()) {
+ // Local bindings need their own field remapping. Only the Correlate
Review Comment:
I don't see how this comment relates to the code.
##########
core/src/main/java/org/apache/calcite/sql2rel/CorrelationReferences.java:
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.calcite.sql2rel;
+
+import org.apache.calcite.plan.RelOptUtil;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.RelVisitor;
+import org.apache.calcite.rel.core.Correlate;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.core.Filter;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rex.RexCorrelVariable;
+import org.apache.calcite.rex.RexFieldAccess;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.rex.RexSubQuery;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableSet;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Set;
+
+/** Conservatively analyzes references to correlation bindings outside a
Review Comment:
what is the result of the analysis?
##########
core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java:
##########
@@ -202,99 +187,77 @@ private static boolean isDirectFieldAccess(RexNode node,
CorrelationId id) {
return builder.build();
}
- /**
- * Traverses a plan and finds all simply correlated row expressions with the
specified id.
- */
- private static Set<RexNode> findCorrelationDependentCalls(CorrelationId
corrId, RelNode plan) {
- SimpleCorrelationCollector finder = new SimpleCorrelationCollector(corrId);
- plan.accept(new RelHomogeneousShuttle() {
- @Override public RelNode visit(RelNode other) {
- // Don't descend into a nested correlate that reuses this id: the refs
below belong to it.
- if (other instanceof Correlate
- && ((Correlate) other).getCorrelationId().equals(corrId)) {
- return other;
+ /** Applies the visitor within the specified correlation scope. */
+ private static RelNode visitCorrelationScope(RelNode root, CorrelationId id,
+ RexShuttle visitor) {
+ return root.accept(new RelHomogeneousShuttle() {
+ @Override public RelNode visit(RelNode rel) {
+ if (rel.getVariablesSet().contains(id)) {
+ if (rel instanceof Correlate) {
+ return visitChild(rel, 0, rel.getInput(0));
+ }
+ if (rel instanceof Project || rel instanceof Filter) {
+ return visitChildren(rel);
+ }
+ return rel;
}
- if (other instanceof Project || other instanceof Filter) {
- other.accept(finder);
- }
- return super.visit(other);
- }
- });
- return finder.correlations;
- }
-
- /**
- * Replaces all row expressions in the plan using the provided mapping.
- *
- * @param plan the relational expression on which we want to perform the
replacements.
- * @param mapping a mapping defining how to replace row expressions in the
plan
- * @return a new relational expression where all expressions present in the
mapping are replaced.
- */
- private static RelNode replaceExpressionsUsingMap(RelNode plan, Map<RexNode,
RexNode> mapping) {
- CallReplacer replacer = new CallReplacer(mapping);
- return plan.accept(new RelHomogeneousShuttle() {
- @Override public RelNode visit(RelNode other) {
- RelNode mNode = super.visitChildren(other);
- return mNode.accept(replacer);
+ final RelNode node = rel instanceof Project || rel instanceof Filter
+ ? rel.accept(visitor) : rel;
+ return visitChildren(node);
}
});
}
- /**
- * A collector of simply correlated row expressions.
+ /** Selects outermost deterministic computations over one correlation.
Review Comment:
what happens to the selected ones? How are they represented?
##########
core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java:
##########
@@ -202,99 +187,77 @@ private static boolean isDirectFieldAccess(RexNode node,
CorrelationId id) {
return builder.build();
}
- /**
- * Traverses a plan and finds all simply correlated row expressions with the
specified id.
- */
- private static Set<RexNode> findCorrelationDependentCalls(CorrelationId
corrId, RelNode plan) {
- SimpleCorrelationCollector finder = new SimpleCorrelationCollector(corrId);
- plan.accept(new RelHomogeneousShuttle() {
- @Override public RelNode visit(RelNode other) {
- // Don't descend into a nested correlate that reuses this id: the refs
below belong to it.
- if (other instanceof Correlate
- && ((Correlate) other).getCorrelationId().equals(corrId)) {
- return other;
+ /** Applies the visitor within the specified correlation scope. */
Review Comment:
is this notion of a "correlation scope" defined?
If not, maybe you can define it here.
--
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]
