You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "somandal (via GitHub)" <gi...@apache.org> on 2023/03/21 04:14:16 UTC
[GitHub] [pinot] somandal opened a new pull request, #10449: Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
somandal opened a new pull request, #10449:
URL: https://github.com/apache/pinot/pull/10449
This PR introduces the multi-stage runtime changes to support Phase 1 of Window Functions, specifically targeting queries of the type `OVER(ORDER BY)` and `OVER(PARTITION BY key1 ORDER BY key2)`. Runtime support has been added for the following classes of window queries:
- `OVER(ORDER BY)` - single and multiple `OVER(ORDER BY)` in the same query
- `OVER(PARTITION BY key1 ORDER BY key2)` - single and multiple `OVER(PARTITION BY key1 ORDER BY key2)` using the same `PARTITION BY` key and `ORDER BY` key across all `OVER()s` in the same query
The window functions supported as part of Phase 1 are: SUM, AVG, MIN, MAX, COUNT, BOOL_OR, and BOOL_AND.
PostgreSQL supports many aggregation functions inside window functions as mentioned in their [documentation](https://www.postgresql.org/docs/current/functions-window.html):
> In addition to these functions, any built-in or user-defined ordinary aggregate (i.e., not ordered-set or hypothetical-set aggregates) can be used as a window function; see Section 9.21 for a list of the built-in aggregates. Aggregate functions act as window functions only when an OVER clause follows the call; otherwise they act as plain aggregates and return a single row for the entire set.
Design document: https://docs.google.com/document/d/13CmFm4djI09JKF_Xty5acoXxJoxC9CLXAsmikgzgtIs/edit?usp=sharing
Window functions Issue: https://github.com/apache/pinot/issues/7213
Window Function Planner PR: https://github.com/apache/pinot/pull/10228
Window Functions Runtime PR for empty OVER() and OVER(PARTITION BY): https://github.com/apache/pinot/pull/10286
Adding ordering as a part of the SortExchange: https://github.com/apache/pinot/pull/10408
This PR does not include support for:
- Custom frames
- Other window functions related to rank and values
- Multiple window groups (basically multiple OVER clauses with different PARTITION BY, ORDER BY and/or FRAME specifications, if these specifications are the same they get grouped into a single window group)
The above will be part of future changes for window function support
**Note:** One issue was found while working on this PR related to Apache Calcite parsing. Details can be found in [this section of the design document](https://docs.google.com/document/d/13CmFm4djI09JKF_Xty5acoXxJoxC9CLXAsmikgzgtIs/edit#heading=h.s6az8ttq3x5f). Look for heading **"Calcite Parser Exception on ORDER BY with > 1 column using default frame"**
cc @siddharthteotia @walterddr @vvivekiyer @ankitsultana
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10449:
URL: https://github.com/apache/pinot/pull/10449#discussion_r1147762453
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/WindowAggregateOperator.java:
##########
@@ -239,16 +238,18 @@ private boolean consumeInputBlocks() {
return true;
}
+ Key emptyOrderKey = AggregationUtils.extractEmptyKey();
List<Object[]> container = block.getContainer();
for (Object[] row : container) {
_numRows++;
- // TODO: Revisit the aggregation logic once ORDER BY inside OVER() support is added, also revisit null direction
- // handling for all query types
+ // TODO: Revisit null direction handling for all query types
Key key = AggregationUtils.extractRowKey(row, _groupSet);
+ Key orderKey = (CollectionUtils.isEmpty(_orderSetInfo.getOrderSet()) || _orderSetInfo.isPartitionByOnly())
Review Comment:
same here. please dont do checks inside row-loop
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10449:
URL: https://github.com/apache/pinot/pull/10449#discussion_r1147792552
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -2165,6 +2165,24 @@
"sql": "EXPLAIN PLAN FOR SELECT MIN(a.col3) OVER(ORDER BY a.col3 RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM a",
"expectedException": "Error explain query plan for.*"
},
+ {
Review Comment:
Nope, I haven't made any additional planner side changes. We had already added planner side support for `ORDER BY`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10449:
URL: https://github.com/apache/pinot/pull/10449#issuecomment-1477363048
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10449?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10449](https://codecov.io/gh/apache/pinot/pull/10449?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (73140b5) into [master](https://codecov.io/gh/apache/pinot/commit/3687b2b732be22dc59f6ba08f90bce9e8b4ee45b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3687b2b) will **decrease** coverage by `49.32%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10449 +/- ##
=============================================
- Coverage 63.23% 13.92% -49.32%
+ Complexity 5905 237 -5668
=============================================
Files 2036 2007 -29
Lines 110973 109340 -1633
Branches 16892 16703 -189
=============================================
- Hits 70177 15224 -54953
- Misses 35606 92886 +57280
+ Partials 5190 1230 -3960
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.92% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...uery/runtime/operator/WindowAggregateOperator.java](https://codecov.io/gh/apache/pinot/pull/10449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9XaW5kb3dBZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (-91.67%)` | :arrow_down: |
| [...query/runtime/operator/utils/AggregationUtils.java](https://codecov.io/gh/apache/pinot/pull/10449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci91dGlscy9BZ2dyZWdhdGlvblV0aWxzLmphdmE=) | `0.00% <0.00%> (-96.50%)` | :arrow_down: |
... and [1833 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10449/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10449:
URL: https://github.com/apache/pinot/pull/10449#discussion_r1147762062
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/WindowAggregateOperator.java:
##########
@@ -239,16 +238,18 @@ private boolean consumeInputBlocks() {
return true;
}
+ Key emptyOrderKey = AggregationUtils.extractEmptyKey();
Review Comment:
i dont think we need to extract this on every block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #10449:
URL: https://github.com/apache/pinot/pull/10449#issuecomment-1483090178
> this PR assumes the mailbox receive will generate ordered block appropriately, correct? if so could we please state so in the javadoc of window aggregate operator?
Thanks for the review! Yes, this PR assumes that mailbox receive will generate ordered blocks. Added a note in the javadocs about this. I've also addressed all other comments. PTAL when you get a chance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10449:
URL: https://github.com/apache/pinot/pull/10449#discussion_r1147816723
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/WindowAggregateOperator.java:
##########
@@ -239,16 +238,18 @@ private boolean consumeInputBlocks() {
return true;
}
+ Key emptyOrderKey = AggregationUtils.extractEmptyKey();
Review Comment:
good catch, fixed
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/WindowAggregateOperator.java:
##########
@@ -239,16 +238,18 @@ private boolean consumeInputBlocks() {
return true;
}
+ Key emptyOrderKey = AggregationUtils.extractEmptyKey();
List<Object[]> container = block.getContainer();
for (Object[] row : container) {
_numRows++;
- // TODO: Revisit the aggregation logic once ORDER BY inside OVER() support is added, also revisit null direction
- // handling for all query types
+ // TODO: Revisit null direction handling for all query types
Key key = AggregationUtils.extractRowKey(row, _groupSet);
+ Key orderKey = (CollectionUtils.isEmpty(_orderSetInfo.getOrderSet()) || _orderSetInfo.isPartitionByOnly())
Review Comment:
good catch, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10449:
URL: https://github.com/apache/pinot/pull/10449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10449: [multistage] Initial (phase 1) Query runtime for window functions with ORDER BY within the OVER() clause
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10449:
URL: https://github.com/apache/pinot/pull/10449#discussion_r1147759188
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -2165,6 +2165,24 @@
"sql": "EXPLAIN PLAN FOR SELECT MIN(a.col3) OVER(ORDER BY a.col3 RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM a",
"expectedException": "Error explain query plan for.*"
},
+ {
Review Comment:
do we have any non-exception test cases we want to enable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org