mihaibudiu commented on code in PR #5385:
URL: https://github.com/apache/calcite/pull/5385#discussion_r4449355604
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -464,9 +497,48 @@ public RelNode removeCorrelationViaRule(RelNode root) {
* {@link org.apache.calcite.rel.core.Correlate}s might be removable in such
way).
*/
public RelNode removeCorrelationViaRule(RelNode root, RuleSet ruleSet) {
+ root = prepareCorrelates(root);
return applyHepProgram(root, ruleSetToHepProgram(ruleSet));
}
+ /** Removes unused RHS computations before matching rules or building frames.
+ * The checked input must also be the input used by the rewrite.
+ */
+ private RelNode prepareCorrelates(RelNode root) {
+ final RelNode prepared = root.accept(new RelHomogeneousShuttle() {
Review Comment:
This shuttle looks important enough to be in its own class with proper
javadoc.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -464,9 +497,48 @@ public RelNode removeCorrelationViaRule(RelNode root) {
* {@link org.apache.calcite.rel.core.Correlate}s might be removable in such
way).
*/
public RelNode removeCorrelationViaRule(RelNode root, RuleSet ruleSet) {
+ root = prepareCorrelates(root);
return applyHepProgram(root, ruleSetToHepProgram(ruleSet));
}
+ /** Removes unused RHS computations before matching rules or building frames.
+ * The checked input must also be the input used by the rewrite.
+ */
+ private RelNode prepareCorrelates(RelNode root) {
+ final RelNode prepared = root.accept(new RelHomogeneousShuttle() {
+ @Override public RelNode visit(RelNode other) {
+ final RelNode rel = super.visit(other);
+ if (!(rel instanceof Correlate)) {
+ return rel;
+ }
+ final Correlate correlate = (Correlate) rel;
+ RelNode right = correlate.getRight();
+ if (!isDeterministic(right)) {
+ final RelBuilder trimBuilder =
relBuilderFactory().create(right.getCluster(), null);
+ final RelNode trimmed = new RelFieldTrimmer(null,
trimBuilder).trim(right);
Review Comment:
Is this the best way to do it?
How about trimming fields in a global pass before you even start the
decorrelation?
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -464,9 +497,48 @@ public RelNode removeCorrelationViaRule(RelNode root) {
* {@link org.apache.calcite.rel.core.Correlate}s might be removable in such
way).
*/
public RelNode removeCorrelationViaRule(RelNode root, RuleSet ruleSet) {
+ root = prepareCorrelates(root);
return applyHepProgram(root, ruleSetToHepProgram(ruleSet));
}
+ /** Removes unused RHS computations before matching rules or building frames.
+ * The checked input must also be the input used by the rewrite.
+ */
+ private RelNode prepareCorrelates(RelNode root) {
+ final RelNode prepared = root.accept(new RelHomogeneousShuttle() {
+ @Override public RelNode visit(RelNode other) {
+ final RelNode rel = super.visit(other);
+ if (!(rel instanceof Correlate)) {
+ return rel;
+ }
+ final Correlate correlate = (Correlate) rel;
+ RelNode right = correlate.getRight();
+ if (!isDeterministic(right)) {
+ final RelBuilder trimBuilder =
relBuilderFactory().create(right.getCluster(), null);
+ final RelNode trimmed = new RelFieldTrimmer(null,
trimBuilder).trim(right);
+ if (isDeterministic(trimmed)) {
+ right = trimmed;
+ }
+ }
+ if (right == ((Correlate) other).getRight()) {
+ return rel;
+ }
+ // Trimming can also remove references to the correlation variable.
+ final ImmutableBitSet requiredColumns =
Review Comment:
should this functionality be part of the trimmer?
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1490,10 +1571,20 @@ private static boolean
isNonNegativeIntegralLiteral(@Nullable RexNode node) {
requireNonNull(mapNewInputToOutputs.get(newInput),
() -> "mapNewInputToOutputs.get(" + newInput + ")");
- RelNode distinct = relBuilder.push(newInput)
+ RelNode projected = relBuilder.push(newInput)
.project(relBuilder.fields(positions))
- .distinct()
.build();
+ if (!isDeterministic(projected)) {
+ // Only values and row-producing operations needed by the parameter
+ // domain are repeated. Unused volatile projections need not prevent
Review Comment:
does "volatile" mean "non-deterministic"?
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1595,10 +1686,80 @@ private RelNode findInputRel(CorRef corVar) {
return getCorRel(corVar);
}
+ /** Checks whether this input can be repeated or shared during decorrelation.
+ * Ordinary table scans assume stable contents within a query; unknown
+ * operators are not assumed deterministic.
+ */
+ private boolean isDeterministic(RelNode rel) {
Review Comment:
But again, as we discussed elsewhere, there are two kinds of determinism:
RAND() and NOW(). The latter can actually be duplicated. That problem still
hasn't been fixed.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1595,10 +1686,80 @@ private RelNode findInputRel(CorRef corVar) {
return getCorRel(corVar);
}
+ /** Checks whether this input can be repeated or shared during decorrelation.
+ * Ordinary table scans assume stable contents within a query; unknown
+ * operators are not assumed deterministic.
+ */
+ private boolean isDeterministic(RelNode rel) {
+ rel = rel.stripped();
+ try {
+ // These operators have expressions outside accept(RexShuttle).
+ if (rel instanceof Aggregate) {
+ for (AggregateCall call : ((Aggregate) rel).getAggCallList()) {
+ if (!call.getAggregation().isDeterministic()) {
+ return false;
+ }
+ call.rexList.forEach(RelDecorrelator::checkDeterministicExpression);
+ }
+ } else if (rel instanceof Window) {
+ for (Window.Group group : ((Window) rel).groups) {
+
group.aggCalls.forEach(RelDecorrelator::checkDeterministicExpression);
+ checkDeterministicExpression(group.lowerBound.getOffset());
+ checkDeterministicExpression(group.upperBound.getOffset());
+ }
+ } else if (rel instanceof Match) {
+ final Match match = (Match) rel;
+ checkDeterministicExpression(match.getPattern());
+ checkDeterministicExpression(match.getAfter());
+ checkDeterministicExpression(match.getInterval());
+
match.getMeasures().values().forEach(RelDecorrelator::checkDeterministicExpression);
+ match.getPatternDefinitions().values()
+ .forEach(RelDecorrelator::checkDeterministicExpression);
+ } else if (rel instanceof AsofJoin) {
+ checkDeterministicExpression(((AsofJoin) rel).getMatchCondition());
+ } else if (rel instanceof ConditionalCorrelate) {
+ checkDeterministicExpression(((ConditionalCorrelate)
rel).getCondition());
+ } else if (!(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)) {
+ return false;
Review Comment:
this is a strange way to do it.
I would think that these all return true, and the default is 'false'. It's
easier to understand.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -1595,10 +1686,80 @@ private RelNode findInputRel(CorRef corVar) {
return getCorRel(corVar);
}
+ /** Checks whether this input can be repeated or shared during decorrelation.
+ * Ordinary table scans assume stable contents within a query; unknown
+ * operators are not assumed deterministic.
+ */
+ private boolean isDeterministic(RelNode rel) {
Review Comment:
This looks like a very useful function. This should be public someplace so
other passes can reuse it.
--
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]
