From df3edfa0427d01a3078a4f659b1a0e001cb78a49 Mon Sep 17 00:00:00 2001 From: jaystarshot Date: Mon, 20 Nov 2023 08:22:44 -0800 Subject: [PATCH] Remove SPI dependency + nits --- .../facebook/presto/SystemSessionProperties.java | 4 ++-- .../presto/sql/planner/BasePlanFragmenter.java | 2 +- .../presto/sql/planner/RelationPlanner.java | 5 +++-- .../planner/optimizations/LogicalCteOptimizer.java | 5 +++-- .../planner/optimizations/PhysicalCteOptimizer.java | 5 +++-- .../presto/sql/planner/planPrinter/PlanPrinter.java | 2 +- presto-spi/pom.xml | 2 +- .../facebook/presto/spi/plan/CteConsumerNode.java | 13 +++++++++---- .../facebook/presto/spi/plan/CteProducerNode.java | 11 ++++++++--- .../facebook/presto/spi/plan/CteReferenceNode.java | 11 ++++++++--- 10 files changed, 39 insertions(+), 21 deletions(-) diff --git a/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java b/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java index e2a08ba9b501..00c0491ffb3b 100644 --- a/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java +++ b/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java @@ -2294,9 +2294,9 @@ public static DataSize getFilterAndProjectMinOutputPageSize(Session session) return session.getSystemProperty(FILTER_AND_PROJECT_MIN_OUTPUT_PAGE_SIZE, DataSize.class); } - public static boolean isMaterializeAllCtes(Session session) + public static CteMaterializationStrategy getCteMaterializationStrategy(Session session) { - return session.getSystemProperty(CTE_MATERIALIZATION_STRATEGY, CteMaterializationStrategy.class).equals(CteMaterializationStrategy.ALL); + return session.getSystemProperty(CTE_MATERIALIZATION_STRATEGY, CteMaterializationStrategy.class); } public static int getFilterAndProjectMinOutputPageRowCount(Session session) diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/BasePlanFragmenter.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/BasePlanFragmenter.java index f662e773db69..70cc1e3efde7 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/BasePlanFragmenter.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/BasePlanFragmenter.java @@ -207,7 +207,7 @@ public PlanNode visitSequence(SequenceNode node, RewriteContext= 1, "Sequence Node has 0 cte producers"); + checkArgument(n >= 1, "Sequence Node has 0 cte producers"); PlanNode source = node.getCteProducers().get(n - 1); FragmentProperties childProperties = new FragmentProperties(new PartitioningScheme( Partitioning.create(SINGLE_DISTRIBUTION, ImmutableList.of()), 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 adbfb7557cfb..473ce6d0bbd2 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 @@ -107,8 +107,8 @@ import java.util.Set; import java.util.stream.IntStream; +import static com.facebook.presto.SystemSessionProperties.getCteMaterializationStrategy; import static com.facebook.presto.SystemSessionProperties.getQueryAnalyzerTimeout; -import static com.facebook.presto.SystemSessionProperties.isMaterializeAllCtes; import static com.facebook.presto.common.type.TypeUtils.isEnumType; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; import static com.facebook.presto.spi.StandardErrorCode.QUERY_PLANNING_TIMEOUT; @@ -118,6 +118,7 @@ import static com.facebook.presto.sql.analyzer.ExpressionTreeUtils.getSourceLocation; import static com.facebook.presto.sql.analyzer.ExpressionTreeUtils.isEqualComparisonExpression; import static com.facebook.presto.sql.analyzer.ExpressionTreeUtils.resolveEnumLiteral; +import static com.facebook.presto.sql.analyzer.FeaturesConfig.CteMaterializationStrategy.ALL; import static com.facebook.presto.sql.analyzer.SemanticExceptions.notSupportedException; import static com.facebook.presto.sql.planner.PlannerUtils.newVariable; import static com.facebook.presto.sql.planner.TranslateExpressionsUtil.toRowExpression; @@ -186,7 +187,7 @@ protected RelationPlan visitTable(Table node, SqlPlannerContext context) context.getNestedCteStack().push(cteName, namedQuery.getQuery()); RelationPlan subPlan = process(namedQuery.getQuery(), context); context.getNestedCteStack().pop(namedQuery.getQuery()); - boolean shouldBeMaterialized = isMaterializeAllCtes(session) && isCteMaterializable(subPlan.getRoot().getOutputVariables()); + boolean shouldBeMaterialized = getCteMaterializationStrategy(session).equals(ALL) && isCteMaterializable(subPlan.getRoot().getOutputVariables()); session.getCteInformationCollector().addCTEReference(cteName, namedQuery.isFromView(), shouldBeMaterialized); if (shouldBeMaterialized) { subPlan = new RelationPlan( diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java index f10cfe14a4f2..01858dac2b1c 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java @@ -39,8 +39,9 @@ import java.util.Optional; import java.util.Stack; -import static com.facebook.presto.SystemSessionProperties.isMaterializeAllCtes; +import static com.facebook.presto.SystemSessionProperties.getCteMaterializationStrategy; import static com.facebook.presto.common.type.BigintType.BIGINT; +import static com.facebook.presto.sql.analyzer.FeaturesConfig.CteMaterializationStrategy.ALL; import static com.google.common.base.Preconditions.checkArgument; /* @@ -79,7 +80,7 @@ public LogicalCteOptimizer(Metadata metadata) @Override public PlanOptimizerResult optimize(PlanNode plan, Session session, TypeProvider types, VariableAllocator variableAllocator, PlanNodeIdAllocator idAllocator, WarningCollector warningCollector) { - if (!isMaterializeAllCtes(session) + if (!getCteMaterializationStrategy(session).equals(ALL) || session.getCteInformationCollector().getCTEInformationList().stream().noneMatch(CTEInformation::isMaterialized)) { return PlanOptimizerResult.optimizerResult(plan, false); } diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PhysicalCteOptimizer.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PhysicalCteOptimizer.java index 8c69732a89c5..8f498b03410b 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PhysicalCteOptimizer.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PhysicalCteOptimizer.java @@ -45,14 +45,15 @@ import java.util.Map; import java.util.Optional; +import static com.facebook.presto.SystemSessionProperties.getCteMaterializationStrategy; import static com.facebook.presto.SystemSessionProperties.getHashPartitionCount; import static com.facebook.presto.SystemSessionProperties.getPartitioningProviderCatalog; -import static com.facebook.presto.SystemSessionProperties.isMaterializeAllCtes; import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED; import static com.facebook.presto.sql.TemporaryTableUtil.assignPartitioningVariables; import static com.facebook.presto.sql.TemporaryTableUtil.assignTemporaryTableColumnNames; import static com.facebook.presto.sql.TemporaryTableUtil.createTemporaryTableScan; import static com.facebook.presto.sql.TemporaryTableUtil.createTemporaryTableWriteWithoutExchanges; +import static com.facebook.presto.sql.analyzer.FeaturesConfig.CteMaterializationStrategy.ALL; import static com.facebook.presto.sql.planner.optimizations.CteUtils.getCtePartitionIndex; import static com.google.common.collect.ImmutableList.toImmutableList; import static java.lang.String.format; @@ -86,7 +87,7 @@ public PhysicalCteOptimizer(Metadata metadata) @Override public PlanOptimizerResult optimize(PlanNode plan, Session session, TypeProvider types, VariableAllocator variableAllocator, PlanNodeIdAllocator idAllocator, WarningCollector warningCollector) { - if (!isMaterializeAllCtes(session) + if (!getCteMaterializationStrategy(session).equals(ALL) || session.getCteInformationCollector().getCTEInformationList().stream().noneMatch(CTEInformation::isMaterialized)) { return PlanOptimizerResult.optimizerResult(plan, false); } diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java index c9e5f0bf5476..44fcf13ae3ab 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java @@ -1397,7 +1397,7 @@ public NodeRepresentation addNode(PlanNode rootNode, String name, String identif public static String getCteExecutionOrder(SequenceNode node) { List cteProducers = node.getCteProducers().stream() - .peek(c -> checkArgument(c instanceof CteProducerNode, "Child of sequence node is not an instance of CteProducerNode")) + .filter(c -> (c instanceof CteProducerNode)) .map(CteProducerNode.class::cast) .collect(Collectors.toList()); if (cteProducers.isEmpty()) { diff --git a/presto-spi/pom.xml b/presto-spi/pom.xml index ebf249a7c916..9508e216874b 100644 --- a/presto-spi/pom.xml +++ b/presto-spi/pom.xml @@ -88,7 +88,7 @@ com.google.guava guava - provided + test diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteConsumerNode.java b/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteConsumerNode.java index d7647cb70117..b0b6a9e5ea7f 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteConsumerNode.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteConsumerNode.java @@ -17,15 +17,13 @@ import com.facebook.presto.spi.relation.VariableReferenceExpression; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import com.google.common.collect.ImmutableList; import javax.annotation.concurrent.Immutable; +import java.util.Collections; import java.util.List; import java.util.Optional; -import static com.google.common.base.Preconditions.checkArgument; - @Immutable public final class CteConsumerNode extends PlanNode @@ -59,7 +57,7 @@ public CteConsumerNode( public List getSources() { // CteConsumer should be the leaf node - return ImmutableList.of(); + return Collections.emptyList(); } @Override @@ -92,4 +90,11 @@ public String getCteName() { return cteName; } + + private static void checkArgument(boolean condition, String message) + { + if (!condition) { + throw new IllegalArgumentException(message); + } + } } diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteProducerNode.java b/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteProducerNode.java index bdf88bc44d59..45666da8be56 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteProducerNode.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteProducerNode.java @@ -17,14 +17,12 @@ import com.facebook.presto.spi.relation.VariableReferenceExpression; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import com.google.common.collect.Iterables; import javax.annotation.concurrent.Immutable; import java.util.List; import java.util.Optional; -import static com.google.common.base.Preconditions.checkArgument; import static java.util.Collections.singletonList; @Immutable @@ -86,7 +84,7 @@ public List getOutputVariables() public PlanNode replaceChildren(List newChildren) { checkArgument(newChildren.size() == 1, "expected newChildren to contain 1 node"); - return new CteProducerNode(newChildren.get(0).getSourceLocation(), getId(), getStatsEquivalentPlanNode(), Iterables.getOnlyElement(newChildren), + return new CteProducerNode(newChildren.get(0).getSourceLocation(), getId(), getStatsEquivalentPlanNode(), newChildren.get(0), cteName, rowCountVariable, originalOutputVariables); } @@ -111,4 +109,11 @@ public VariableReferenceExpression getRowCountVariable() { return rowCountVariable; } + + private static void checkArgument(boolean condition, String message) + { + if (!condition) { + throw new IllegalArgumentException(message); + } + } } diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteReferenceNode.java b/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteReferenceNode.java index eb9ffe0c50fd..bce849a2bc7c 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteReferenceNode.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/plan/CteReferenceNode.java @@ -17,14 +17,12 @@ import com.facebook.presto.spi.relation.VariableReferenceExpression; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import com.google.common.collect.Iterables; import javax.annotation.concurrent.Immutable; import java.util.List; import java.util.Optional; -import static com.google.common.base.Preconditions.checkArgument; import static java.util.Collections.singletonList; @Immutable @@ -78,7 +76,7 @@ public List getOutputVariables() public PlanNode replaceChildren(List newChildren) { checkArgument(newChildren.size() == 1, "expected newChildren to contain 1 node"); - return new CteReferenceNode(newChildren.get(0).getSourceLocation(), getId(), getStatsEquivalentPlanNode(), Iterables.getOnlyElement(newChildren), cteName); + return new CteReferenceNode(newChildren.get(0).getSourceLocation(), getId(), getStatsEquivalentPlanNode(), newChildren.get(0), cteName); } @Override @@ -97,4 +95,11 @@ public String getCteName() { return cteName; } + + private static void checkArgument(boolean condition, String message) + { + if (!condition) { + throw new IllegalArgumentException(message); + } + } }