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

Remove redundant ORDER BY from WITH and views #18159

Merged
merged 3 commits into from
Aug 4, 2023
Merged

Remove redundant ORDER BY from WITH and views #18159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b581474
Clarify analyzer invocations
martint Jul 7, 2023
6409506
Add missing scope for merge
martint Jul 7, 2023
e651e6f
Remove redundant ORDER BY in WITH and views
martint Jul 7, 2023
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
3 changes: 1 addition & 2 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/Analyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")) {
Expand Down
46 changes: 27 additions & 19 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider getting rid of this method and making the caller explicitly say top level or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this method implies it's not top-level (since there's an outer query scope). It's the method below that is not necessary. I'll update its usages and remove it.

{
return analyze(node, Optional.of(outerQueryScope));
return analyze(node, Optional.of(outerQueryScope), false);
}

public Scope analyze(Node node, Optional<Scope> outerQueryScope)
private Scope analyze(Node node, Optional<Scope> 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<Scope> outerQueryScope, UpdateKind updateKind)
{
return new Visitor(outerQueryScope, warningCollector, Optional.of(updateKind))
return new Visitor(outerQueryScope, warningCollector, Optional.of(updateKind), true)
.process(relation, Optional.empty());
}

Expand All @@ -487,15 +492,17 @@ private enum UpdateKind
private final class Visitor
extends AstVisitor<Scope, Optional<Scope>>
{
private final boolean isTopLevel;
private final Optional<Scope> outerQueryScope;
private final WarningCollector warningCollector;
private final Optional<UpdateKind> updateKind;

private Visitor(Optional<Scope> outerQueryScope, WarningCollector warningCollector, Optional<UpdateKind> updateKind)
private Visitor(Optional<Scope> outerQueryScope, WarningCollector warningCollector, Optional<UpdateKind> 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
Expand Down Expand Up @@ -538,7 +545,7 @@ protected Scope visitInsert(Insert insert, Optional<Scope> 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);
Expand Down Expand Up @@ -920,7 +927,7 @@ protected Scope visitCreateTableAsSelect(CreateTableAsSelect node, Optional<Scop
accessControl.checkCanCreateTable(session.toSecurityContext(), targetTable, explicitlySetProperties);

// analyze the query that creates the table
Scope queryScope = analyze(node.getQuery(), createScope(scope));
Scope queryScope = analyze(node.getQuery());

ImmutableList.Builder<ColumnMetadata> columns = ImmutableList.builder();

Expand Down Expand Up @@ -996,7 +1003,7 @@ protected Scope visitCreateView(CreateView node, Optional<Scope> 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);

Expand Down Expand Up @@ -1199,7 +1206,7 @@ protected Scope visitTableExecute(TableExecute node, Optional<Scope> scope)
}
}

Scope tableScope = analyze(table, scope);
Scope tableScope = analyze(table);

String catalogName = tableName.getCatalogName();
CatalogHandle catalogHandle = getRequiredCatalogHandle(metadata, session, node, catalogName);
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -1491,7 +1498,7 @@ protected Scope visitQuery(Query node, Optional<Scope> 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"));
Expand Down Expand Up @@ -1577,7 +1584,7 @@ else if (expressionType instanceof MapType) {
protected Scope visitLateral(Lateral node, Optional<Scope> 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());
}

Expand Down Expand Up @@ -2955,7 +2962,7 @@ else if (base instanceof AliasedRelation aliasedRelation &&
protected Scope visitTableSubquery(TableSubquery node, Optional<Scope> 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());
}

Expand Down Expand Up @@ -2986,7 +2993,7 @@ protected Scope visitQuerySpecification(QuerySpecification node, Optional<Scope>

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"));
Expand Down Expand Up @@ -3398,9 +3405,10 @@ protected Scope visitMerge(Merge merge, Optional<Scope> 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()) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -4898,7 +4906,7 @@ private Scope analyzeWith(Query node, Optional<Scope> 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()) {
Expand Down
58 changes: 58 additions & 0 deletions core/trino-main/src/test/java/io/trino/sql/planner/TestOrderBy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")))))))));
}
}