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 index 48e84c3838aa..2a6d1b1aebc4 100644 --- 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) 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) + { + 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) { - 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) @Override protected Scope visitExcept(Except node, Optional 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 index 332ff6e1cbe1..ad248639a5c2 100644 --- 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 index c717eb4a43a6..414b535ba5d4 100644 --- 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 index e6a74473664e..e9d979cd7b2e 100644 --- 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 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 index 946eda6e9832..5910f5c504ef 100644 --- 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 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 distinct) { this(Optional.of(location), left, right, distinct); } - private Except(Optional location, Relation left, Relation right, boolean distinct) + private Except(Optional location, Relation left, Relation right, Optional 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 index 171d93fe56cb..b1956adbb920 100644 --- 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 relations; - public Intersect(List relations, boolean distinct) + public Intersect(List relations, Optional distinct) { this(Optional.empty(), relations, distinct); } - public Intersect(NodeLocation location, List relations, boolean distinct) + public Intersect(NodeLocation location, List relations, Optional distinct) { this(Optional.of(location), relations, distinct); } - private Intersect(Optional location, List relations, boolean distinct) + private Intersect(Optional location, List relations, Optional 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 index ca9f0d123671..ba5c030b18d6 100644 --- 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 distinct; - protected SetOperation(Optional location, boolean distinct) + protected SetOperation(Optional location, Optional distinct) { super(location); - this.distinct = distinct; + this.distinct = requireNonNull(distinct, "distinct is null"); } - public boolean isDistinct() + public Optional 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 index 6421f01ad8a8..25218abc4df8 100644 --- 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 relations; - public Union(List relations, boolean distinct) + public Union(List relations, Optional distinct) { this(Optional.empty(), relations, distinct); } - public Union(NodeLocation location, List relations, boolean distinct) + public Union(NodeLocation location, List relations, Optional distinct) { this(Optional.of(location), relations, distinct); } - private Union(Optional location, List relations, boolean distinct) + private Union(Optional location, List relations, Optional 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 index 322733120eeb..f38bfaf44dc2 100644 --- 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 index d2c5d52973d5..81c68d018c51 100644 --- 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()); }