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.
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
Do not plan unnecessary Sort #818
Conversation
Examples: ORDER BY at top-levelEXPLAIN
WITH t(x) AS (VALUES 1,2,3)
SELECT * FROM t
ORDER BY x
Unnecessary ORDER BY in subquery queryEXPLAIN
WITH t(x) AS (VALUES 1,2,3)
SELECT * FROM (
SELECT * FROM t
ORDER BY x
)
Necessary ORDER BY in subqueryEXPLAIN
WITH t(x) AS (VALUES 1,2,3)
SELECT * FROM (
SELECT * FROM t
ORDER BY x
LIMIT 1
)
Unnecessary ORDER BY in scalar subqueryEXPLAIN
WITH t(x) AS (VALUES 1,2,3)
SELECT (SELECT x FROM t ORDER BY x)
FROM (VALUES 10)
Necessary ORDER BY in scalar subqueryEXPLAIN
WITH t(x) AS (VALUES 1,2,3)
SELECT (SELECT x FROM t ORDER BY x LIMIT 1)
FROM (VALUES 10)
Unnecessary ORDER BY in aggregation queryEXPLAIN
WITH t(x) AS (VALUES 1,2,3)
SELECT array_agg(x)
FROM (
SELECT * FROM t
ORDER BY x
)
Necessary ORDER BY in UNION ALL queryEXPLAIN
VALUES 1
UNION ALL
VALUES 2
ORDER BY 1
Unnecessary ORDER BY in UNION ALL queryEXPLAIN
SELECT * FROM (
VALUES 1
UNION ALL
VALUES 2
ORDER BY 1
)
|
e6a4b4f
to
a23f858
Compare
fd1d0ff
to
9c6f62f
Compare
presto-main/src/main/java/io/prestosql/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
if (sourceScope.getOuterQueryParent().isPresent() && !node.getLimit().isPresent() && !node.getOffset().isPresent()) { | ||
// not the root scope and ORDER BY is ineffective | ||
analysis.markRedundantOrderBy(orderBy); | ||
warningCollector.add(new PrestoWarning(REDUNDANT_ORDER_BY, "ORDER BY in subquery has no effect")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
presto-main/src/main/java/io/prestosql/sql/planner/QueryPlanner.java
Outdated
Show resolved
Hide resolved
@@ -121,6 +121,7 @@ | |||
private boolean preferPartialAggregation = true; | |||
private boolean optimizeTopNRowNumber = true; | |||
private boolean workProcessorPipelines; | |||
private boolean removeRedundantSort = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added getter/setter last, but this isn't the last field. This class is messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's think again about example that you also mentioned in your blog:
SELECT *, row_number() OVER ()
FROM (SELECT * FROM nation ORDER BY name DESC)
the proper way of writing this is:
SELECT *, row_number() OVER (ORDER BY name DESC)
FROM nation
Is the proper way a drop-in replacement?
Before the change, the old way would use distributed sort.
Does the standard-compliant version use distributed sort too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the proper way is not equally performant yet, we shouldn't have the feature on by default just yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we added distributed sort specifically for the unpartitioned window function case, so I think that should be the case. I’ll double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added getter/setter last, but this isn't the last field.
I added it right below "work processor pipelines" in both cases. The other ones are out of order.
This class is messy.
100% agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the proper way a drop-in replacement?
Before the change, the old way would use distributed sort.
Regardless, a query like this is just broken, semantically speaking. Almost every case I've seen is people relying on ORDER BY for aggregation queries, since 1) we didn't use to support ordered aggregates until sometime last year, 2) window functions have always supported ORDER BY.
For anyone relying on that behavior, there's the fallback of disabling the config flag (for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out we currently don't run distributed sorts for unpartitioned window functions with an ORDER BY clause. Here's a PR to add it: #821
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we added distributed sort specifically for the unpartitioned window function case, so I think that should be the case. I’ll double check
Distributed sort utilizes entire cluster memory, while WindowOperator
does not (it collects partition rows).
Keep in mind that unordered row_number window function was replaced with RowNumberNode
operator (See: io.prestosql.sql.planner.optimizations.WindowFilterPushDown.Rewriter#canReplaceWithRowNumber
). This means that such hacky:
SELECT *, row_number() OVER ()
FROM (SELECT * FROM nation ORDER BY name DESC)
query utilizes distributed sort and skips WindowOperator
completely. However, for:
SELECT *, row_number() OVER (ORDER BY name DESC)
FROM nation
It will still execute WindowOperator
which will collect all partition data (thus mitigating benefits of distributed sort).
Maybe we should improve WindowFilterPushDown
rule too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, for this shape:
SELECT *, row_number() OVER ()
FROM (SELECT * FROM nation ORDER BY name DESC)
even if the ORDER BY weren't removed, there's no guarantee that the row_number() function (or any other window function, for that matter) will not load all the data in memory and shuffle it in some way before producing its results. So it could break at any time for other reasons unrelated to this optimization, so I'm not too concerned about it, especially since we have an escape hatch.
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for "Clean up analysis of ORDER BY" commit which I didn't review.
The calls to analysis.setOrderByExpressions() were spread out across the caller and the analyzeOrderBy methods. This puts them in a single place, so it's easier to reason about. Also, inline unnecessary methods
According to the SQL specification, ORDER BY is only meaningful for the immediate query expression that contains it. Since the only operations that are affected by ORDER BY are FETCH FIRST/LIMIT and OFFSET, an ORDER BY clause in a subquery has no effect. With this change, the analyzer marks those ORDER BY clauses as redundant and the planner skips adding a Sort node. If a redundant ORDER BY is present, the analyzer will emit a warning. To ease the transition for users that may rely on the old behavior, the new behavior can be disabled via a session property or config option.
Cherry-pick of trinodb/trino#818 Co-author: Martin Traverso <mtraverso@gmail.com>
Cherry-pick of trinodb/trino#818 Co-author: Martin Traverso <mtraverso@gmail.com>
Cherry-pick of trinodb/trino#818 Co-author: Martin Traverso <mtraverso@gmail.com>
Avoid planning unnecessary sort operations. This fixes #759 and is an alternative to #810.