Add warning message for UNION queries without ALL/DISTINCT keyword

Co-authored-by: Sundeep Katepalli
prestodb · Aug 14, 2020 · 27e922f · 27e922f
1 parent c6c34d6
commit 27e922f
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 32 deletions.
diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java
@@ -1243,7 +1243,7 @@ protected Scope visitSetOperation(SetOperation node, Optional<Scope> scope)
         private boolean isExpensiveUnionDistinct(SetOperation setOperation, Type[] outputTypes)
         {
             return setOperation instanceof Union &&
-                    setOperation.isDistinct() &&
+                    setOperation.isDistinct().orElse(false) &&
                     outputTypes.length > UNION_DISTINCT_FIELDS_WARNING_THRESHOLD &&
                     Arrays.stream(outputTypes)
                             .anyMatch(
@@ -1254,10 +1254,22 @@ private boolean isExpensiveUnionDistinct(SetOperation setOperation, Type[] outpu
                                             type instanceof RowType);
         }
 
+        @Override
+        protected Scope visitUnion(Union node, Optional<Scope> scope)
+        {
+            if (!node.isDistinct().isPresent()) {
+                warningCollector.add(new PrestoWarning(
+                        PERFORMANCE_WARNING,
+                        "UNION specified without ALL or DISTINCT keyword is equivalent to UNION DISTINCT, which is computationally expensive. " +
+                                "Consider using UNION ALL when possible, or specifically add the keyword DISTINCT if absolutely necessary"));
+            }
+            return visitSetOperation(node, scope);
+        }
+
         @Override
         protected Scope visitIntersect(Intersect node, Optional<Scope> scope)
         {
-            if (!node.isDistinct()) {
+            if (!node.isDistinct().orElse(true)) {
                 throw new SemanticException(NOT_SUPPORTED, node, "INTERSECT ALL not yet implemented");
             }
 
@@ -1267,7 +1279,7 @@ protected Scope visitIntersect(Intersect node, Optional<Scope> scope)
         @Override
         protected Scope visitExcept(Except node, Optional<Scope> scope)
         {
-            if (!node.isDistinct()) {
+            if (!node.isDistinct().orElse(true)) {
                 throw new SemanticException(NOT_SUPPORTED, node, "EXCEPT ALL not yet implemented");
             }
 

diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java
@@ -801,7 +801,7 @@ protected RelationPlan visitUnion(Union node, Void context)
         SetOperationPlan setOperationPlan = process(node);
 
         PlanNode planNode = new UnionNode(idAllocator.getNextId(), setOperationPlan.getSources(), setOperationPlan.getOutputVariables(), setOperationPlan.getVariableMapping());
-        if (node.isDistinct()) {
+        if (node.isDistinct().orElse(true)) {
             planNode = distinct(planNode);
         }
         return new RelationPlan(planNode, analysis.getScope(node), planNode.getOutputVariables());

diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java b/presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java
@@ -499,9 +499,7 @@ protected Void visitUnion(Union node, Integer indent)
 
                 if (relations.hasNext()) {
                     builder.append("UNION ");
-                    if (!node.isDistinct()) {
-                        builder.append("ALL ");
-                    }
+                    node.isDistinct().map(distinct -> distinct ? builder.append("DISTINCT ") : builder.append("ALL "));
                 }
             }
 
@@ -514,9 +512,7 @@ protected Void visitExcept(Except node, Integer indent)
             processRelation(node.getLeft(), indent);
 
             builder.append("EXCEPT ");
-            if (!node.isDistinct()) {
-                builder.append("ALL ");
-            }
+            node.isDistinct().map(distinct -> distinct ? builder.append("DISTINCT ") : builder.append("ALL "));
 
             processRelation(node.getRight(), indent);
 
@@ -533,9 +529,7 @@ protected Void visitIntersect(Intersect node, Integer indent)
 
                 if (relations.hasNext()) {
                     builder.append("INTERSECT ");
-                    if (!node.isDistinct()) {
-                        builder.append("ALL ");
-                    }
+                    node.isDistinct().map(distinct -> distinct ? builder.append("DISTINCT ") : builder.append("ALL "));
                 }
             }
 

diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java b/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java
@@ -749,7 +749,15 @@ public Node visitSetOperation(SqlBaseParser.SetOperationContext context)
         QueryBody left = (QueryBody) visit(context.left);
         QueryBody right = (QueryBody) visit(context.right);
 
-        boolean distinct = context.setQuantifier() == null || context.setQuantifier().DISTINCT() != null;
+        Optional<Boolean> distinct = Optional.empty();
+        if (context.setQuantifier() != null) {
+            if (context.setQuantifier().DISTINCT() != null) {
+                distinct = Optional.of(true);
+            }
+            else if (context.setQuantifier().ALL() != null) {
+                distinct = Optional.of(false);
+            }
+        }
 
         switch (context.operator.getType()) {
             case SqlBaseLexer.UNION:

diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/tree/Except.java b/presto-parser/src/main/java/com/facebook/presto/sql/tree/Except.java
@@ -28,17 +28,17 @@ public class Except
     private final Relation left;
     private final Relation right;
 
-    public Except(Relation left, Relation right, boolean distinct)
+    public Except(Relation left, Relation right, Optional<Boolean> distinct)
     {
         this(Optional.empty(), left, right, distinct);
     }
 
-    public Except(NodeLocation location, Relation left, Relation right, boolean distinct)
+    public Except(NodeLocation location, Relation left, Relation right, Optional<Boolean> distinct)
     {
         this(Optional.of(location), left, right, distinct);
     }
 
-    private Except(Optional<NodeLocation> location, Relation left, Relation right, boolean distinct)
+    private Except(Optional<NodeLocation> location, Relation left, Relation right, Optional<Boolean> distinct)
     {
         super(location, distinct);
         requireNonNull(left, "left is null");

diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/tree/Intersect.java b/presto-parser/src/main/java/com/facebook/presto/sql/tree/Intersect.java
@@ -27,17 +27,17 @@ public class Intersect
 {
     private final List<Relation> relations;
 
-    public Intersect(List<Relation> relations, boolean distinct)
+    public Intersect(List<Relation> relations, Optional<Boolean> distinct)
     {
         this(Optional.empty(), relations, distinct);
     }
 
-    public Intersect(NodeLocation location, List<Relation> relations, boolean distinct)
+    public Intersect(NodeLocation location, List<Relation> relations, Optional<Boolean> distinct)
     {
         this(Optional.of(location), relations, distinct);
     }
 
-    private Intersect(Optional<NodeLocation> location, List<Relation> relations, boolean distinct)
+    private Intersect(Optional<NodeLocation> location, List<Relation> relations, Optional<Boolean> distinct)
     {
         super(location, distinct);
         requireNonNull(relations, "relations is null");

diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/tree/SetOperation.java b/presto-parser/src/main/java/com/facebook/presto/sql/tree/SetOperation.java
@@ -16,18 +16,20 @@
 import java.util.List;
 import java.util.Optional;
 
+import static java.util.Objects.requireNonNull;
+
 public abstract class SetOperation
         extends QueryBody
 {
-    private final boolean distinct;
+    private final Optional<Boolean> distinct;
 
-    protected SetOperation(Optional<NodeLocation> location, boolean distinct)
+    protected SetOperation(Optional<NodeLocation> location, Optional<Boolean> distinct)
     {
         super(location);
-        this.distinct = distinct;
+        this.distinct = requireNonNull(distinct, "distinct is null");
     }
 
-    public boolean isDistinct()
+    public Optional<Boolean> isDistinct()
     {
         return distinct;
     }

diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/tree/Union.java b/presto-parser/src/main/java/com/facebook/presto/sql/tree/Union.java
@@ -27,17 +27,17 @@ public class Union
 {
     private final List<Relation> relations;
 
-    public Union(List<Relation> relations, boolean distinct)
+    public Union(List<Relation> relations, Optional<Boolean> distinct)
     {
         this(Optional.empty(), relations, distinct);
     }
 
-    public Union(NodeLocation location, List<Relation> relations, boolean distinct)
+    public Union(NodeLocation location, List<Relation> relations, Optional<Boolean> distinct)
     {
         this(Optional.of(location), relations, distinct);
     }
 
-    private Union(Optional<NodeLocation> location, List<Relation> relations, boolean distinct)
+    private Union(Optional<NodeLocation> location, List<Relation> relations, Optional<Boolean> distinct)
     {
         super(location, distinct);
         requireNonNull(relations, "relations is null");

diff --git a/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java b/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java
@@ -486,9 +486,9 @@ public void testIntersect()
                 new Query(
                         Optional.empty(),
                         new Intersect(ImmutableList.of(
-                                new Intersect(ImmutableList.of(createSelect123(), createSelect123()), true),
+                                new Intersect(ImmutableList.of(createSelect123(), createSelect123()), Optional.of(true)),
                                 createSelect123()
-                        ), false),
+                        ), Optional.of(false)),
                         Optional.empty(),
                         Optional.empty()));
     }
@@ -500,9 +500,9 @@ public void testUnion()
                 new Query(
                         Optional.empty(),
                         new Union(ImmutableList.of(
-                                new Union(ImmutableList.of(createSelect123(), createSelect123()), true),
+                                new Union(ImmutableList.of(createSelect123(), createSelect123()), Optional.of(true)),
                                 createSelect123()
-                        ), false),
+                        ), Optional.of(false)),
                         Optional.empty(),
                         Optional.empty()));
     }

diff --git a/presto-tests/src/test/java/com/facebook/presto/execution/TestWarnings.java b/presto-tests/src/test/java/com/facebook/presto/execution/TestWarnings.java
@@ -29,6 +29,7 @@
 import static com.facebook.presto.SessionTestUtils.TEST_SESSION;
 import static com.facebook.presto.execution.TestQueryRunnerUtil.createQueryRunner;
 import static com.facebook.presto.spi.StandardWarningCode.PARSER_WARNING;
+import static com.facebook.presto.spi.StandardWarningCode.PERFORMANCE_WARNING;
 import static com.facebook.presto.spi.StandardWarningCode.TOO_MANY_STAGES;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Sets.difference;
@@ -64,7 +65,7 @@ public void testStageCountWarningThreshold()
                     .append(stageIndex);
         }
         String query = queryBuilder.toString();
-        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(TOO_MANY_STAGES.toWarningCode()));
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(TOO_MANY_STAGES.toWarningCode(), PERFORMANCE_WARNING.toWarningCode()));
         assertWarnings(queryRunner, TEST_SESSION, noWarningsQuery, ImmutableSet.of());
     }