Add dynamic filter canonicalization in UnaliasSymbolReferences

prestodb · Aug 23, 2020 · ad31ac0 · ad31ac0
1 parent 0cfa23f
commit ad31ac0
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 4 deletions.
diff --git a/.../src/main/java/com/facebook/presto/sql/planner/optimizations/UnaliasSymbolReferences.java b/.../src/main/java/com/facebook/presto/sql/planner/optimizations/UnaliasSymbolReferences.java
@@ -527,6 +527,8 @@ public PlanNode visitJoin(JoinNode node, RewriteContext<Void> context)
             Optional<VariableReferenceExpression> canonicalLeftHashVariable = canonicalize(node.getLeftHashVariable());
             Optional<VariableReferenceExpression> canonicalRightHashVariable = canonicalize(node.getRightHashVariable());
 
+            Map<String, VariableReferenceExpression> canonicalDynamicFilters = canonicalizeAndDistinct(node.getDynamicFilters());
+
             if (node.getType().equals(INNER)) {
                 canonicalCriteria.stream()
                         .filter(clause -> clause.getLeft().getType().equals(clause.getRight().getType()))
@@ -545,7 +547,7 @@ public PlanNode visitJoin(JoinNode node, RewriteContext<Void> context)
                     canonicalLeftHashVariable,
                     canonicalRightHashVariable,
                     node.getDistributionType(),
-                    node.getDynamicFilters());
+                    canonicalDynamicFilters);
         }
 
         @Override
@@ -764,6 +766,19 @@ private List<VariableReferenceExpression> canonicalizeAndDistinct(List<VariableR
             return builder.build();
         }
 
+        private Map<String, VariableReferenceExpression> canonicalizeAndDistinct(Map<String, VariableReferenceExpression> dynamicFilters)
+        {
+            Set<VariableReferenceExpression> added = new HashSet<>();
+            ImmutableMap.Builder<String, VariableReferenceExpression> builder = ImmutableMap.builder();
+            for (Map.Entry<String, VariableReferenceExpression> entry : dynamicFilters.entrySet()) {
+                VariableReferenceExpression canonical = canonicalize(entry.getValue());
+                if (added.add(canonical)) {
+                    builder.put(entry.getKey(), canonical);
+                }
+            }
+            return builder.build();
+        }
+
         private WindowNode.Specification canonicalizeAndDistinct(WindowNode.Specification specification)
         {
             return new WindowNode.Specification(

diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/plan/JoinNode.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/plan/JoinNode.java
@@ -27,6 +27,7 @@
 
 import javax.annotation.concurrent.Immutable;
 
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -124,7 +125,11 @@ public JoinNode(@JsonProperty("id") PlanNodeId id,
         }
 
         for (VariableReferenceExpression variableReferenceExpression : dynamicFilters.values()) {
-            checkArgument(right.getOutputVariables().contains(variableReferenceExpression), "Right join input doesn't contain symbol for dynamic filter: %s", variableReferenceExpression);
+            checkArgument(right.getOutputVariables().contains(variableReferenceExpression), format(
+                    "Right join input doesn't contain symbol for dynamic filter: %s, rightVariables: %s, dynamicFilters.values(): %s",
+                    variableReferenceExpression,
+                    Arrays.toString(right.getOutputVariables().toArray()),
+                    Arrays.toString(dynamicFilters.values().toArray())));
         }
     }
 

diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/DynamicFiltersChecker.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/DynamicFiltersChecker.java
@@ -32,6 +32,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
@@ -41,6 +42,7 @@
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Sets.difference;
 import static com.google.common.collect.Sets.intersection;
+import static java.lang.String.format;
 
 /**
  * When dynamic filter assignments are present on a Join node, they should be consumed by a Filter node on its probe side
@@ -99,7 +101,12 @@ private Set<String> extractUnmatchedDynamicFilters(AbstractJoinNode node, Void c
 
                 Set<String> consumedBuildSide = node.getBuild().accept(this, context);
                 Set<String> unconsumedByBuildSide = intersection(currentJoinDynamicFilters, consumedBuildSide);
-                verify(unconsumedByBuildSide.isEmpty(), "Dynamic filters %s present in join were consumed by its build side.", unconsumedByBuildSide);
+                verify(unconsumedByBuildSide.isEmpty(),
+                        format(
+                                "Dynamic filters %s present in join were consumed by its build side. consumedBuildSide %s, currentJoinDynamicFilters %s",
+                                Arrays.toString(unconsumedByBuildSide.toArray()),
+                                Arrays.toString(consumedBuildSide.toArray()),
+                                Arrays.toString(currentJoinDynamicFilters.toArray())));
 
                 Set<String> unmatched = new HashSet<>(consumedBuildSide);
                 unmatched.addAll(consumedProbeSide);

diff --git a/...-main/src/test/java/com/facebook/presto/sql/planner/sanity/TestDynamicFiltersChecker.java b/...-main/src/test/java/com/facebook/presto/sql/planner/sanity/TestDynamicFiltersChecker.java
@@ -98,7 +98,7 @@ public void testUnconsumedDynamicFilterInJoin()
         validatePlan(root);
     }
 
-    @Test(expectedExceptions = VerifyException.class, expectedExceptionsMessageRegExp = "Dynamic filters \\[DF\\] present in join were consumed by its build side.")
+    @Test(expectedExceptions = VerifyException.class, expectedExceptionsMessageRegExp = "Dynamic filters \\[DF\\] present in join were consumed by its build side. consumedBuildSide \\[DF\\], currentJoinDynamicFilters \\[DF\\]")
     public void testDynamicFilterConsumedOnBuildSide()
     {
         PlanNode root = builder.join(