Skip to content

Conversation

@yashmayya
Copy link
Contributor

  • Fixes [Multi Stage] Add ROWS support for aggregation window functions #11406.
  • Background reading - https://www.postgresql.org/docs/current/tutorial-window.html, https://www.postgresql.org/docs/current/functions-window.html, https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-WINDOW-FUNCTIONS (this one is most relevant to this PR).
  • Currently, Pinot's window function implementations have limited or even incorrect support for window frame bounds. For instance, FIRST_VALUE / LAST_VALUE assume that the window frame is always ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING even though the default window frame as per standard SQL is RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW. Furthermore, support for defining the lower bound explicitly as UNBOUNDED PRECEDING / CURRENT ROW / n FOLLOWING / n PRECEDING and the upper bound as UNBOUNDED FOLLOWING / CURRENT ROW / n FOLLOWING / n PRECEDING does not exist.
  • This patch adds support for any custom bounds (offset based or otherwise) for ROWS window frames, and also adds support for UNBOUNDED PRECEDING / CURRENT ROW / UNBOUNDED FOLLOWING bounds for RANGE window frames. There are a ton of edge cases to be handled here but this patch attempts to add test cases to cover most of these scenarios.
  • Note that Calcite (and hence Pinot) only supports ROWS and RANGE based window frame bounds, whereas Postgres also supports GROUPS.
  • The planner side changes (mainly literal extraction) are built over [WIP] Support preceding and following in WINDOW #14233.
  • Apart from the need to add support for offset bounds for RANGE based window frames, another important future enhancement is to optimize the performance of ROWS based window frames for aggregate window functions where both the lower and upper bounds are offset based / current row. Since the changes in this patch are built over the existing framework for window functions where a "merger" is used to merge values for aggregate window functions, it isn't possible to use a sliding window based algorithm to efficiently compute aggregates for windows. This will require more significant changes to the framework but is critical to ensure performant computations especially for larger windows. Optimizations have been added in this patch to ensure that aggregation window functions over window frames with UNBOUNDED PRECEDING lower bound or UNBOUNDED FOLLOWING upper bound are computed efficiently.
  • Note that all the changes here only affect the aggregate window functions (SUM, COUNT, MIN, MAX etc.) and FIRST_VALUE / LAST_VALUE. The other window functions currently supported by Pinot (LAG, LEAD, RANK, DENSE_RANK, ROW_NUMBER) don't support custom window frame bounds and Calcite ensures that during query planning.
  • Calcite also does some other validation for window frame bounds like making sure lower bound isn't UNBOUNDED FOLLOWING / upper bound isn't UNBOUNDED PRECEDING, lower bound isn't UNBOUNDED FOLLOWING if upper bound is UNBOUNDED PRECEDING and vice versa etc.

@yashmayya yashmayya added feature release-notes Referenced by PRs that need attention when compiling the next release notes multi-stage Related to the multi-stage query engine labels Oct 17, 2024
@yashmayya yashmayya force-pushed the window-function-custom-window-frames branch from 1018bcf to 640ad27 Compare October 17, 2024 10:37
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 88.61210% with 32 lines in your changes missing coverage. Please review.

Project coverage is 63.78%. Comparing base (59551e4) to head (5346878).
Report is 1214 commits behind head on master.

Files with missing lines Patch % Lines
.../query/planner/logical/PlanNodeToRelConverter.java 0.00% 11 Missing ⚠️
...ator/window/aggregate/AggregateWindowFunction.java 93.00% 1 Missing and 6 partials ⚠️
...e/rel/rules/PinotWindowExchangeNodeInsertRule.java 88.88% 2 Missing and 4 partials ⚠️
.../query/planner/logical/RelToPlanNodeConverter.java 75.00% 1 Missing and 3 partials ⚠️
...not/query/runtime/operator/window/WindowFrame.java 84.61% 1 Missing and 1 partial ⚠️
...uery/runtime/operator/WindowAggregateOperator.java 80.00% 0 Missing and 1 partial ⚠️
...operator/window/value/LastValueWindowFunction.java 97.14% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14249      +/-   ##
============================================
+ Coverage     61.75%   63.78%   +2.03%     
- Complexity      207     1536    +1329     
============================================
  Files          2436     2627     +191     
  Lines        133233   144844   +11611     
  Branches      20636    22187    +1551     
============================================
+ Hits          82274    92385   +10111     
- Misses        44911    45647     +736     
- Partials       6048     6812     +764     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.75% <88.61%> (+2.04%) ⬆️
java-21 63.67% <88.61%> (+2.04%) ⬆️
skip-bytebuffers-false 63.76% <88.61%> (+2.01%) ⬆️
skip-bytebuffers-true 63.64% <88.61%> (+35.92%) ⬆️
temurin 63.78% <88.61%> (+2.03%) ⬆️
unittests 63.77% <88.61%> (+2.03%) ⬆️
unittests1 55.52% <88.61%> (+8.63%) ⬆️
unittests2 34.27% <1.42%> (+6.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yashmayya yashmayya marked this pull request as ready for review October 17, 2024 11:39
@yashmayya yashmayya force-pushed the window-function-custom-window-frames branch from 640ad27 to a445cb0 Compare October 17, 2024 14:42
Comment on lines -52 to -53
if (_partitionByOnly) {
return processPartitionOnlyRows(rows);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd initially removed this optimization to reduce clutter since there are a lot of different cases being handled in the new function implementation. However, on second thoughts, the optimization to avoid key computation (among other things) for each row might be significant enough to be worth retaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the optimization is still applied to windows without ORDER BY, since Calcite forces the window frame to be RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING for such windows (and we do avoid the per row key computation for RANGE/ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING). So the only other case is where the partition keys and order by keys are identical.

@yashmayya yashmayya force-pushed the window-function-custom-window-frames branch 4 times, most recently from f1425c0 to 377fecd Compare October 18, 2024 13:05
@yashmayya yashmayya force-pushed the window-function-custom-window-frames branch from 377fecd to 6e95ca0 Compare October 21, 2024 06:56
@Test
public void testWindowFunctionsWithCustomWindowFrame() {
String queryWithDefaultWindow = "SELECT col1, col2, RANK() OVER (PARTITION BY col1 ORDER BY col2) FROM a";
_queryEnvironment.planQuery(queryWithDefaultWindow);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a complete test for a query ? The expectation is that planning wont throw an exception ?

Also the same test contains queries that will throw a parse exception ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a complete test for a query

The queries aren't actually executed, just validated, compiled and optimized.

The expectation is that planning wont throw an exception ?

Yes. I can change it to an assertion on QueryEnvironment.canCompileQuery to make that more clear perhaps.

Also the same test contains queries that will throw a parse exception ?

Yes.

}

@Test
public void testWindowFunction()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is added to check if queries with these window functions execute ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah basically to verify the end to end flow (query planning + execution with runtime operators) works without errors.

@yashmayya
Copy link
Contributor Author

Superseded by #14273.

@yashmayya yashmayya closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature multi-stage Related to the multi-stage query engine release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Multi Stage] Add ROWS support for aggregation window functions

3 participants