Revert "Enable logical property propagation by default"

This reverts commit 4b453a5.
prestodb · Mar 12, 2024 · 4d6b143 · 4d6b143
1 parent 635d2d2
commit 4d6b143
Show file tree

Hide file tree

Showing 8 changed files with 20 additions and 75 deletions.
diff --git a/presto-docs/src/main/sphinx/admin/properties.rst b/presto-docs/src/main/sphinx/admin/properties.rst
@@ -803,15 +803,6 @@ Optimizer Properties
 
     Plan canonicalization strategies used to canonicalize a query plan for history based optimization.
 
-``optimizer.exploit-constraints``
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-    * **Type:** ``boolean``
-    * **Default value:** ``true``
-
-    Enable analysis and propagation of logical properties (distinct keys, cardinality, etc.) among the nodes of
-    a query plan. The optimizer may then use these properties to perform various optimizations.
-
 
 Planner Properties
 --------------------------------------
@@ -973,4 +964,4 @@ Logging Properties
     * **Type:** ``data size``
     * **Default value:** ``100MB``
 
-    The maximum file size for the log file of the HTTP server.
+    The maximum file size for the log file of the HTTP server.
diff --git a/presto-docs/src/main/sphinx/optimizer.rst b/presto-docs/src/main/sphinx/optimizer.rst
@@ -9,4 +9,3 @@ Query Optimizer
     optimizer/cost-in-explain
     optimizer/cost-based-optimizations
     optimizer/history-based-optimization
-    optimizer/logical-properties
diff --git a/presto-docs/src/main/sphinx/optimizer/logical-properties.rst b/presto-docs/src/main/sphinx/optimizer/logical-properties.rst
diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java
@@ -173,7 +173,7 @@ public class FeaturesConfig
     private double memoryRevokingThreshold = 0.9;
     private boolean parseDecimalLiteralsAsDouble;
     private boolean useMarkDistinct = true;
-    private boolean exploitConstraints = true;
+    private boolean exploitConstraints;
     private boolean preferPartialAggregation = true;
     private PartialAggregationStrategy partialAggregationStrategy = PartialAggregationStrategy.ALWAYS;
     private double partialAggregationByteReductionThreshold = 0.5;

diff --git a/...a/com/facebook/presto/sql/planner/iterative/properties/LogicalPropertiesProviderImpl.java b/...a/com/facebook/presto/sql/planner/iterative/properties/LogicalPropertiesProviderImpl.java
@@ -203,8 +203,12 @@ public LogicalProperties getAggregationProperties(AggregationNode aggregationNod
             throw new IllegalStateException("Expected source PlanNode to be a GroupReference with LogicalProperties");
         }
 
+        if (aggregationNode.getGroupingKeys().isEmpty() && aggregationNode.getAggregations().isEmpty()) {
+            throw new IllegalStateException("Aggregation node with no grouping columns and no aggregation functions");
+        }
+
         LogicalPropertiesImpl sourceProperties = (LogicalPropertiesImpl) ((GroupReference) aggregationNode.getSource()).getLogicalProperties().get();
-        if (aggregationNode.getGroupingKeys().isEmpty()) {
+        if (!aggregationNode.getAggregations().isEmpty() && aggregationNode.getGroupingKeys().isEmpty()) {
             //aggregation with no grouping variables, single row output
             return propagateAndLimitProperties(sourceProperties, Long.valueOf(1));
         }

diff --git a/...java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantAggregateDistinct.java b/...java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantAggregateDistinct.java
@@ -23,11 +23,11 @@
 
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.facebook.presto.spi.plan.AggregationNode.Aggregation.removeDistinct;
 import static com.facebook.presto.sql.planner.plan.Patterns.aggregation;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
 /**
  * Removes distinct from aggregates where the combination of aggregate columns and grouping variables contain a unique key.
@@ -60,18 +60,16 @@ public Result apply(AggregationNode node, Captures captures, Context context)
         ImmutableMap.Builder<VariableReferenceExpression, AggregationNode.Aggregation> aggregationsBuilder = ImmutableMap.builder();
 
         for (Map.Entry<VariableReferenceExpression, AggregationNode.Aggregation> agg : node.getAggregations().entrySet()) {
-            if (node.getGroupingKeys().stream().allMatch(key -> key instanceof VariableReferenceExpression) &&
-                    (agg.getValue()).getArguments().stream().allMatch(arg -> arg instanceof VariableReferenceExpression)) {
-                Set<VariableReferenceExpression> varAndGroupingKeySet =
-                        Stream.concat(node.getGroupingKeys().stream().map(VariableReferenceExpression.class::cast),
-                                        (agg.getValue()).getArguments().stream().map(VariableReferenceExpression.class::cast))
-                                .collect(toImmutableSet());
-                if (agg.getValue().isDistinct() && ((GroupReference) node.getSource()).getLogicalProperties().get().isDistinct(varAndGroupingKeySet)) {
-                    aggregationsBuilder.put(agg.getKey(), removeDistinct(agg.getValue()));
-                    continue;
-                }
+            Set<VariableReferenceExpression> varAndGroupingKeySet =
+                    Stream.concat(node.getGroupingKeys().stream().map(VariableReferenceExpression.class::cast),
+                                    (agg.getValue()).getArguments().stream().map(VariableReferenceExpression.class::cast))
+                            .collect(Collectors.toSet());
+            if (agg.getValue().isDistinct() && ((GroupReference) node.getSource()).getLogicalProperties().get().isDistinct(varAndGroupingKeySet)) {
+                aggregationsBuilder.put(agg.getKey(), removeDistinct(agg.getValue()));
+            }
+            else {
+                aggregationsBuilder.put(agg);
             }
-            aggregationsBuilder.put(agg);
         }
 
         Map<VariableReferenceExpression, AggregationNode.Aggregation> newAggregations = aggregationsBuilder.build();

diff --git a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java b/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java
@@ -158,7 +158,7 @@ public void testDefaults()
                 .setFilterAndProjectMinOutputPageSize(new DataSize(500, KILOBYTE))
                 .setFilterAndProjectMinOutputPageRowCount(256)
                 .setUseMarkDistinct(true)
-                .setExploitConstraints(true)
+                .setExploitConstraints(false)
                 .setPreferPartialAggregation(true)
                 .setPartialAggregationStrategy(PartialAggregationStrategy.ALWAYS)
                 .setPartialAggregationByteReductionThreshold(0.5)
@@ -372,7 +372,7 @@ public void testExplicitPropertyMappings()
                 .put("arrayagg.implementation", "LEGACY")
                 .put("multimapagg.implementation", "LEGACY")
                 .put("optimizer.use-mark-distinct", "false")
-                .put("optimizer.exploit-constraints", "false")
+                .put("optimizer.exploit-constraints", "true")
                 .put("optimizer.prefer-partial-aggregation", "false")
                 .put("optimizer.partial-aggregation-strategy", "automatic")
                 .put("optimizer.partial-aggregation-byte-reduction-threshold", "0.8")
@@ -576,7 +576,7 @@ public void testExplicitPropertyMappings()
                 .setFilterAndProjectMinOutputPageSize(new DataSize(1, MEGABYTE))
                 .setFilterAndProjectMinOutputPageRowCount(2048)
                 .setUseMarkDistinct(false)
-                .setExploitConstraints(false)
+                .setExploitConstraints(true)
                 .setPreferPartialAggregation(false)
                 .setPartialAggregationStrategy(PartialAggregationStrategy.AUTOMATIC)
                 .setPartialAggregationByteReductionThreshold(0.8)

diff --git a/presto-main/src/test/java/com/facebook/presto/sql/planner/TestLogicalPlanner.java b/presto-main/src/test/java/com/facebook/presto/sql/planner/TestLogicalPlanner.java
@@ -1398,21 +1398,6 @@ public void testComplexOrderBy()
                         "    COUNT(*), " +
                         "    SUM(REDUCE(col1, ROW(0),(l, r) -> l, x -> 1)) " +
                         "  )",
-                output(aggregation(ImmutableMap.of(),
-                        values())));
-
-        Session session = Session.builder(this.getQueryRunner().getDefaultSession())
-                .setSystemProperty(EXPLOIT_CONSTRAINTS, Boolean.toString(false))
-                .build();
-        assertDistributedPlan("SELECT COUNT(*) " +
-                        "FROM (values ARRAY['a', 'b']) as t(col1) " +
-                        "ORDER BY " +
-                        "  IF( " +
-                        "    SUM(REDUCE(col1, ROW(0),(l, r) -> l, x -> 1)) > 0, " +
-                        "    COUNT(*), " +
-                        "    SUM(REDUCE(col1, ROW(0),(l, r) -> l, x -> 1)) " +
-                        "  )",
-                session,
                 output(
                         project(
                                 exchange(