Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Enable logical property propagation by default" #22171

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Revert "Enable logical property propagation by default" #22171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions presto-docs/src/main/sphinx/admin/properties.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
--------------------------------------
Expand Down Expand Up @@ -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.
1 change: 0 additions & 1 deletion presto-docs/src/main/sphinx/optimizer.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,3 @@ Query Optimizer
optimizer/cost-in-explain
optimizer/cost-based-optimizations
optimizer/history-based-optimization
optimizer/logical-properties
32 changes: 0 additions & 32 deletions presto-docs/src/main/sphinx/optimizer/logical-properties.rst
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 5 additions & 1 deletion ...a/com/facebook/presto/sql/planner/iterative/properties/LogicalPropertiesProviderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
22 changes: 10 additions & 12 deletions ...java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantAggregateDistinct.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down
6 changes: 3 additions & 3 deletions presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand Down
15 changes: 0 additions & 15 deletions presto-main/src/test/java/com/facebook/presto/sql/planner/TestLogicalPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading