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.

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

Support ORDER BY in window functions #23929

Merged
merged 8 commits into from
Nov 6, 2024
Merged

Conversation

losipiuk
Copy link
Member

@losipiuk losipiuk commented Oct 26, 2024

Add basic support for ORDER BY in aggregate functions used in WINDOW context.
Lightly speaking, not a very performant implementation.

TODO:

  • - test coverage

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## General
* Add support for ORDER BY clause in aggregate functions used in window context. ({issue}`issuenumber`)

@losipiuk
Copy link
Member Author

Still draft (I think mostly tests are missing). But please take a look.

@losipiuk losipiuk force-pushed the lo/listagg-1 branch 2 times, most recently from 931f8a2 to ea7f65f Compare October 28, 2024 12:35
@losipiuk losipiuk marked this pull request as ready for review October 28, 2024 12:35
@losipiuk
Copy link
Member Author

Good for review

@losipiuk losipiuk force-pushed the lo/listagg-1 branch 2 times, most recently from 8f5bd36 to 692b296 Compare October 28, 2024 14:58
@losipiuk losipiuk changed the title Support ORDER BY in window functions (WIP) Support ORDER BY in window functions Oct 28, 2024
@losipiuk
Copy link
Member Author

addressed

Comment on lines -4337 to -4340
if (windowFunction.getOrderBy().isPresent()) {
throw semanticException(NOT_SUPPORTED, windowFunction, "Window function with ORDER BY is not supported");
}

Copy link
Member

Choose a reason for hiding this comment

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

Why not accessible? Is it covered in TestAnalyzer?

@losipiuk losipiuk requested a review from kasiafi November 2, 2024 19:37
@losipiuk
Copy link
Member Author

losipiuk commented Nov 2, 2024

addressed. 🍏 @kasiafi ?

wendigo
wendigo previously requested changes Nov 2, 2024
Copy link
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

🧌

@losipiuk
Copy link
Member Author

losipiuk commented Nov 2, 2024

🧌

look on the bright side. This allowed for one more github contribution to collection :P

@kasiafi
Copy link
Member

kasiafi commented Nov 4, 2024

One more thing:

trino> select array_agg(a ORDER BY last_value(b) OVER()) OVER (partition by c)
    -> from (values (1, 2, 3)) t(a, b, c);

This succeeds by silently ignoring the inner window.

In StatementAnalyzer.analyzeWindowFunctions we need to explicitly check that there are no nested windows in the ORDER BY. For now, the check is done only for the aggregation arguments. It should be generalized to work for ORDER BY and FILTER too.

@losipiuk
Copy link
Member Author

losipiuk commented Nov 5, 2024

One more thing:

trino> select array_agg(a ORDER BY last_value(b) OVER()) OVER (partition by c)
    -> from (values (1, 2, 3)) t(a, b, c);

This succeeds by silently ignoring the inner window.

In StatementAnalyzer.analyzeWindowFunctions we need to explicitly check that there are no nested windows in the ORDER BY. For now, the check is done only for the aggregation arguments. It should be generalized to work for ORDER BY and FILTER too.

For aggregation it does not seem to be handles super well:

trino:tpch> select array_agg(a ORDER BY last_value(b) OVER()) FROM (VALUES (1, 2, 3)) t(a, b, c);
Query 20241105_212042_00026_usqbk failed: Internal error
java.lang.NullPointerException
        at java.base/java.util.Objects.requireNonNull(Objects.java:220)
        at java.base/java.util.Optional.of(Optional.java:113)
        at io.trino.sql.planner.TranslationMap.getSymbolForColumn(TranslationMap.java:1271)
        at io.trino.sql.planner.TranslationMap.translate(TranslationMap.java:643)
        at io.trino.sql.planner.TranslationMap.translate(TranslationMap.java:317)
        at io.trino.sql.planner.TranslationMap.rewrite(TranslationMap.java:298)
        at io.trino.sql.planner.PlanBuilder.translate(PlanBuilder.java:85)
        at io.trino.sql.planner.QueryPlanner.coerce(QueryPlanner.java:2073)
        at io.trino.sql.planner.QueryPlanner.planWindowFunctions(QueryPlanner.java:1477)
        at io.trino.sql.planner.QueryPlanner.plan(QueryPlanner.java:421)
        at io.trino.sql.planner.RelationPlanner.visitQuerySpecification(RelationPlanner.java:1743)
        at io.trino.sql.planner.RelationPlanner.visitQuerySpecification(RelationPlanner.java:217)
        at io.trino.sql.tree.QuerySpecification.accept(QuerySpecification.java:155)
        at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27)
        at io.trino.sql.planner.QueryPlanner.planQueryBody(QueryPlanner.java:1083)
        at io.trino.sql.planner.QueryPlanner.plan(QueryPlanner.java:224)
        at io.trino.sql.planner.RelationPlanner.visitQuery(RelationPlanner.java:1736)
        at io.trino.sql.planner.RelationPlanner.visitQuery(RelationPlanner.java:217)
        at io.trino.sql.tree.Query.accept(Query.java:119)
        at io.trino.sql.tree.AstVisitor.process(AstVisitor.java:27)
        at io.trino.sql.planner.LogicalPlanner.createRelationPlan(LogicalPlanner.java:919)
        at io.trino.sql.planner.LogicalPlanner.planStatementWithoutOutput(LogicalPlanner.java:382)
        at io.trino.sql.planner.LogicalPlanner.planStatement(LogicalPlanner.java:351)
        at io.trino.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:250)
        at io.trino.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:243)
        at io.trino.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:238)
        at io.trino.execution.SqlQueryExecution.doPlanQuery(SqlQueryExecution.java:503)
        at io.trino.execution.SqlQueryExecution.planQuery(SqlQueryExecution.java:482)
        at io.trino.execution.SqlQueryExecution.start(SqlQueryExecution.java:420)
        at io.trino.execution.SqlQueryManager.createQuery(SqlQueryManager.java:272)
        at io.trino.dispatcher.LocalDispatchQuery.startExecution(LocalDispatchQuery.java:150)
        at io.trino.dispatcher.LocalDispatchQuery.lambda$waitForMinimumWorkers$2(LocalDispatchQuery.java:134)
        at io.airlift.concurrent.MoreFutures.lambda$addSuccessCallback$12(MoreFutures.java:570)
        at io.airlift.concurrent.MoreFutures$3.onSuccess(MoreFutures.java:545)
        at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1137)
        at io.trino.$gen.Trino_testversion____20241105_205752_71.run(Unknown Source)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
        at java.base/java.lang.Thread.run(Thread.java:1575)

@losipiuk
Copy link
Member Author

losipiuk commented Nov 5, 2024

@kasiafi addressed

@losipiuk losipiuk merged commit 8dbf130 into trinodb:master Nov 6, 2024
98 checks passed
@github-actions github-actions bot added this to the 465 milestone Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants