diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/Analyzer.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/Analyzer.java index f08161ff853a..57f6f94f7b8b 100644 --- a/core/trino-main/src/main/java/io/trino/sql/analyzer/Analyzer.java +++ b/core/trino-main/src/main/java/io/trino/sql/analyzer/Analyzer.java @@ -32,7 +32,6 @@ import java.util.List; import java.util.Map; -import java.util.Optional; import static io.trino.spi.StandardErrorCode.EXPRESSION_NOT_SCALAR; import static io.trino.sql.analyzer.ExpressionTreeUtils.extractAggregateFunctions; @@ -94,7 +93,7 @@ public Analysis analyze(Statement statement, QueryType queryType) StatementAnalyzer analyzer = statementAnalyzerFactory.createStatementAnalyzer(analysis, session, warningCollector, CorrelationSupport.ALLOWED); try (var ignored = scopedSpan(tracer, "analyze")) { - analyzer.analyze(rewrittenStatement, Optional.empty()); + analyzer.analyze(rewrittenStatement); } try (var ignored = scopedSpan(tracer, "access-control")) { diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java index 594e9c74f12b..fced580c36da 100644 --- a/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java +++ b/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java @@ -455,20 +455,25 @@ class StatementAnalyzer this.correlationSupport = requireNonNull(correlationSupport, "correlationSupport is null"); } + public Scope analyze(Node node) + { + return analyze(node, Optional.empty(), true); + } + public Scope analyze(Node node, Scope outerQueryScope) { - return analyze(node, Optional.of(outerQueryScope)); + return analyze(node, Optional.of(outerQueryScope), false); } - public Scope analyze(Node node, Optional outerQueryScope) + private Scope analyze(Node node, Optional outerQueryScope, boolean isTopLevel) { - return new Visitor(outerQueryScope, warningCollector, Optional.empty()) + return new Visitor(outerQueryScope, warningCollector, Optional.empty(), isTopLevel) .process(node, Optional.empty()); } public Scope analyzeForUpdate(Relation relation, Optional outerQueryScope, UpdateKind updateKind) { - return new Visitor(outerQueryScope, warningCollector, Optional.of(updateKind)) + return new Visitor(outerQueryScope, warningCollector, Optional.of(updateKind), true) .process(relation, Optional.empty()); } @@ -487,15 +492,17 @@ private enum UpdateKind private final class Visitor extends AstVisitor> { + private final boolean isTopLevel; private final Optional outerQueryScope; private final WarningCollector warningCollector; private final Optional updateKind; - private Visitor(Optional outerQueryScope, WarningCollector warningCollector, Optional updateKind) + private Visitor(Optional outerQueryScope, WarningCollector warningCollector, Optional updateKind, boolean isTopLevel) { this.outerQueryScope = requireNonNull(outerQueryScope, "outerQueryScope is null"); this.warningCollector = requireNonNull(warningCollector, "warningCollector is null"); this.updateKind = requireNonNull(updateKind, "updateKind is null"); + this.isTopLevel = isTopLevel; } @Override @@ -538,7 +545,7 @@ protected Scope visitInsert(Insert insert, Optional scope) } // analyze the query that creates the data - Scope queryScope = analyze(insert.getQuery(), createScope(scope)); + Scope queryScope = analyze(insert.getQuery()); // verify the insert destination columns match the query RedirectionAwareTableHandle redirection = metadata.getRedirectionAwareTableHandle(session, targetTable); @@ -920,7 +927,7 @@ protected Scope visitCreateTableAsSelect(CreateTableAsSelect node, Optional columns = ImmutableList.builder(); @@ -996,7 +1003,7 @@ protected Scope visitCreateView(CreateView node, Optional scope) // analyze the query that creates the view StatementAnalyzer analyzer = statementAnalyzerFactory.createStatementAnalyzer(analysis, session, warningCollector, CorrelationSupport.ALLOWED); - Scope queryScope = analyzer.analyze(node.getQuery(), scope); + Scope queryScope = analyzer.analyze(node.getQuery()); accessControl.checkCanCreateView(session.toSecurityContext(), viewName); @@ -1199,7 +1206,7 @@ protected Scope visitTableExecute(TableExecute node, Optional scope) } } - Scope tableScope = analyze(table, scope); + Scope tableScope = analyze(table); String catalogName = tableName.getCatalogName(); CatalogHandle catalogHandle = getRequiredCatalogHandle(metadata, session, node, catalogName); @@ -1383,7 +1390,7 @@ protected Scope visitCreateMaterializedView(CreateMaterializedView node, Optiona // analyze the query that creates the view StatementAnalyzer analyzer = statementAnalyzerFactory.createStatementAnalyzer(analysis, session, warningCollector, CorrelationSupport.ALLOWED); - Scope queryScope = analyzer.analyze(node.getQuery(), scope); + Scope queryScope = analyzer.analyze(node.getQuery()); validateColumns(node, queryScope.getRelationType()); @@ -1491,7 +1498,7 @@ protected Scope visitQuery(Query node, Optional scope) if (node.getOrderBy().isPresent()) { orderByExpressions = analyzeOrderBy(node, getSortItemsFromOrderBy(node.getOrderBy()), queryBodyScope); - if (queryBodyScope.getOuterQueryParent().isPresent() && node.getLimit().isEmpty() && node.getOffset().isEmpty()) { + if ((queryBodyScope.getOuterQueryParent().isPresent() || !isTopLevel) && node.getLimit().isEmpty() && node.getOffset().isEmpty()) { // not the root scope and ORDER BY is ineffective analysis.markRedundantOrderBy(node.getOrderBy().get()); warningCollector.add(new TrinoWarning(REDUNDANT_ORDER_BY, "ORDER BY in subquery may have no effect")); @@ -1577,7 +1584,7 @@ else if (expressionType instanceof MapType) { protected Scope visitLateral(Lateral node, Optional scope) { StatementAnalyzer analyzer = statementAnalyzerFactory.createStatementAnalyzer(analysis, session, warningCollector, CorrelationSupport.ALLOWED); - Scope queryScope = analyzer.analyze(node.getQuery(), scope); + Scope queryScope = analyzer.analyze(node.getQuery(), scope.orElseThrow()); return createAndAssignScope(node, scope, queryScope.getRelationType()); } @@ -2955,7 +2962,7 @@ else if (base instanceof AliasedRelation aliasedRelation && protected Scope visitTableSubquery(TableSubquery node, Optional scope) { StatementAnalyzer analyzer = statementAnalyzerFactory.createStatementAnalyzer(analysis, session, warningCollector, CorrelationSupport.ALLOWED); - Scope queryScope = analyzer.analyze(node.getQuery(), scope); + Scope queryScope = analyzer.analyze(node.getQuery(), scope.orElseThrow()); return createAndAssignScope(node, scope, queryScope.getRelationType()); } @@ -2986,7 +2993,7 @@ protected Scope visitQuerySpecification(QuerySpecification node, Optional orderByExpressions = analyzeOrderBy(node, orderBy.getSortItems(), orderByScope.get()); - if (sourceScope.getOuterQueryParent().isPresent() && node.getLimit().isEmpty() && node.getOffset().isEmpty()) { + if ((sourceScope.getOuterQueryParent().isPresent() || !isTopLevel) && node.getLimit().isEmpty() && node.getOffset().isEmpty()) { // not the root scope and ORDER BY is ineffective analysis.markRedundantOrderBy(orderBy); warningCollector.add(new TrinoWarning(REDUNDANT_ORDER_BY, "ORDER BY in subquery may have no effect")); @@ -3398,9 +3405,10 @@ protected Scope visitMerge(Merge merge, Optional scope) throw semanticException(NOT_SUPPORTED, merge, "Cannot merge into a table with check constraints"); } - Scope targetTableScope = analyzer.analyzeForUpdate(relation, scope, UpdateKind.MERGE); - Scope sourceTableScope = process(merge.getSource(), scope); - Scope joinScope = createAndAssignScope(merge, scope, targetTableScope.getRelationType().joinWith(sourceTableScope.getRelationType())); + Scope mergeScope = createScope(scope); + Scope targetTableScope = analyzer.analyzeForUpdate(relation, Optional.of(mergeScope), UpdateKind.MERGE); + Scope sourceTableScope = process(merge.getSource(), mergeScope); + Scope joinScope = createAndAssignScope(merge, Optional.of(mergeScope), targetTableScope.getRelationType().joinWith(sourceTableScope.getRelationType())); for (ColumnSchema column : dataColumnSchemas) { if (accessControl.getColumnMask(session.toSecurityContext(), tableName, column.getName(), column.getType()).isPresent()) { @@ -4567,7 +4575,7 @@ private RelationType analyzeView(Query query, QualifiedObjectName name, Optional StatementAnalyzer analyzer = statementAnalyzerFactory .withSpecializedAccessControl(viewAccessControl) .createStatementAnalyzer(analysis, viewSession, warningCollector, CorrelationSupport.ALLOWED); - Scope queryScope = analyzer.analyze(query, Scope.create()); + Scope queryScope = analyzer.analyze(query); return queryScope.getRelationType().withAlias(name.getObjectName(), null); } catch (RuntimeException e) { @@ -4898,7 +4906,7 @@ private Scope analyzeWith(Query node, Optional scope) if (!isRecursive) { Query query = withQuery.getQuery(); - process(query, withScopeBuilder.build()); + analyze(query, withScopeBuilder.build()); // check if all or none of the columns are explicitly alias if (withQuery.getColumnNames().isPresent()) { diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/TestOrderBy.java b/core/trino-main/src/test/java/io/trino/sql/planner/TestOrderBy.java index f76778d887c9..1cae9996803f 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/TestOrderBy.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/TestOrderBy.java @@ -13,17 +13,28 @@ */ package io.trino.sql.planner; +import com.google.common.collect.ImmutableList; import io.trino.sql.planner.assertions.BasePlanTest; import io.trino.sql.planner.plan.EnforceSingleRowNode; import io.trino.sql.planner.plan.ExchangeNode; +import io.trino.sql.planner.plan.FilterNode; +import io.trino.sql.planner.plan.ProjectNode; +import io.trino.sql.planner.plan.RowNumberNode; import io.trino.sql.planner.plan.SortNode; import io.trino.sql.planner.plan.TopNNode; import io.trino.sql.planner.plan.ValuesNode; import org.junit.jupiter.api.Test; import static io.trino.sql.planner.assertions.PlanMatchPattern.anyTree; +import static io.trino.sql.planner.assertions.PlanMatchPattern.exchange; import static io.trino.sql.planner.assertions.PlanMatchPattern.node; import static io.trino.sql.planner.assertions.PlanMatchPattern.output; +import static io.trino.sql.planner.assertions.PlanMatchPattern.sort; +import static io.trino.sql.planner.assertions.PlanMatchPattern.topN; +import static io.trino.sql.planner.assertions.PlanMatchPattern.values; +import static io.trino.sql.planner.plan.ExchangeNode.Scope.LOCAL; +import static io.trino.sql.tree.SortItem.NullOrdering.LAST; +import static io.trino.sql.tree.SortItem.Ordering.ASCENDING; public class TestOrderBy extends BasePlanTest @@ -94,4 +105,51 @@ public void testRedundantOrderByInUnion() node(ValuesNode.class), node(ValuesNode.class)))); } + + @Test + public void testRedundantOrderByInWith() + { + assertPlan(""" + WITH t(a) AS ( + SELECT * FROM (VALUES 2, 1) t(a) + ORDER BY a) + SELECT * FROM t + """, + output(node(ValuesNode.class))); + } + + @Test + public void testOrderByInWithLimit() + { + assertPlan(""" + WITH t(a) AS ( + SELECT * FROM (VALUES 2, 1) t(a) + ORDER BY a + LIMIT 1) + SELECT * FROM t + """, + output( + topN(1, ImmutableList.of(sort("c", ASCENDING, LAST)), TopNNode.Step.FINAL, + topN(1, ImmutableList.of(sort("c", ASCENDING, LAST)), TopNNode.Step.PARTIAL, + values("c"))))); + } + + @Test + public void testOrderByInWithOffset() + { + assertPlan(""" + WITH t(a) AS ( + SELECT * FROM (VALUES (2),(1)) t(a) + ORDER BY a + OFFSET 1) + SELECT * FROM t + """, + output( + node(ProjectNode.class, + node(FilterNode.class, + node(RowNumberNode.class, + exchange(LOCAL, + sort(exchange(LOCAL, + values("c"))))))))); + } }