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/09/12 18:41:46 UTC
[GitHub] [pinot] somandal opened a new pull request, #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
somandal opened a new pull request, #11577:
URL: https://github.com/apache/pinot/pull/11577
This PR adds support for `ROWS` type window frames for aggregation window functions with an `ORDER BY` clause within the `OVER()`. The default frame type for `ORDER BY` within `OVER()` is `RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW`. This PR adds support for the same default frame boundaries but with `ROWS` type.
Issue: https://github.com/apache/pinot/issues/11406
Example queries this can support:
- SELECT AVG(a.col1) OVER(ORDER BY a.col2 ROWS UNBOUNDED PRECEDING) from table;
- SELECT COUNT(a.col3) OVER (PARTITION BY a.col1 ORDER BY a.col2 ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) from table;
This also allows the use of `ROW_NUMBER()` (which only allows `ROWS` type frames) with aggregation functions, such as:
- SELECT ROW_NUMBER() OVER (PARTITION BY a.col1 ORDER BY a.col2), SUM(a.col3) OVER (PARTITION BY a.col1 ORDER BY a.col2 ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) from table;
The supported functions with this change are:
- COUNT
- SUM
- AVG
- MIN
- MAX
- BOOL_AND
- BOOL_OR
- ROW_NUMBER (this was [already supported](https://github.com/apache/pinot/pull/10587), no new changes in this PR for this)
--
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 pull request #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11577:
URL: https://github.com/apache/pinot/pull/11577#issuecomment-1725972870
> I usually let the test run and look at the difference in the plans and verify whether the planner side makes sense. At least during development of planner side code I've found this very useful to catch bugs or find opportunities for improvements. Unfortunately I agree that this is a very tedious process to maintain this long list of plans and keep modifying them for any planner side changes. I don't know of a good way to generate these automatically though (unlike runtime where for most queries H2 can be used). Would it be helpful to reduce duplication in the plans by removing some of the unnecessary ones (e.g. in window functions initially we had tests for single window functions and corresponding tests using multiple window functions - this was good for the initial development to ensure both work but may not be so useful now)?
one way is to utilize something similar to https://github.com/apache/calcite/blob/9c758942ab51af22a1095087ea2daeabc7d692ea/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml where
1. the RulesTest.xml is generated and compared against the reference during build/test time; and
2. once we validate all the deltas look good, we can directly copy-paste over the changes from the generated xml to the ref xml
3. create the PR then merged/checked-in the ref --> so there's no hand modification required
thoughts?
--
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
Re: [PR] [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal closed pull request #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
URL: https://github.com/apache/pinot/pull/11577
--
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 #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #11577:
URL: https://github.com/apache/pinot/pull/11577#issuecomment-1725840184
> Out of the scope of this PR, can you share how you usually generate the expected result for the query plan test? We have added so many hardcoded plan tests, and whenever we need to add a feature that changes the query plan it is extremely painful because we need to modify thousands of lines of test. #11418 is an example where the actual change might be 20 lines but almost 2000 lines of test change. We need to either modify the test to only test the part related, or provide a way to regenerate the expected results automatically
I usually let the test run and look at the difference in the plans and verify whether the planner side makes sense. At least during development of planner side code I've found this very useful to catch bugs or find opportunities for improvements. Unfortunately I agree that this is a very tedious process to maintain this long list of plans and keep modifying them for any planner side changes. I don't know of a good way to generate these automatically though (unlike runtime where for most queries H2 can be used). Would it be helpful to reduce duplication in the plans by removing some of the unnecessary ones (e.g. in window functions initially we had tests for single window functions and corresponding tests using multiple window functions - this was good for the initial development to ensure both work but may not be so useful now)?
--
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
Re: [PR] [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #11577:
URL: https://github.com/apache/pinot/pull/11577#issuecomment-3050344875
Closing this as it's a really old PR and we'll mostly need to revisit all this anyways
--
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 #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11577:
URL: https://github.com/apache/pinot/pull/11577#issuecomment-1716291672
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11577?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11577](https://app.codecov.io/gh/apache/pinot/pull/11577?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (60f0b4c) into [master](https://app.codecov.io/gh/apache/pinot/commit/bc0d9fb65916bb87fa5272e94007a94a7ffdc569?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bc0d9fb) will **decrease** coverage by `48.58%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11577 +/- ##
=============================================
- Coverage 63.05% 14.47% -48.58%
+ Complexity 1108 201 -907
=============================================
Files 2325 2325
Lines 124797 124797
Branches 19056 19057 +1
=============================================
- Hits 78693 18068 -60625
- Misses 40508 105200 +64692
+ Partials 5596 1529 -4067
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.47% <0.00%> (-48.45%)` | :arrow_down: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.47% <0.00%> (-48.58%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.47% <0.00%> (-48.58%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11577/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.47% <0.00%> (-0.01%)` | :arrow_down: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11577?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [...uery/runtime/operator/WindowAggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11577?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9XaW5kb3dBZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (-96.00%)` | :arrow_down: |
... and [1497 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11577/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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 #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #11577:
URL: https://github.com/apache/pinot/pull/11577#discussion_r1326228139
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -1238,6 +1238,19 @@
"\n"
]
},
+ {
Review Comment:
OVER() or OVER() with only PARTITION BY both only take default frame as "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING/CURRENT ROW". Calcite throws an exception if you modify this to be of RANGE type saying an ORDER BY clause must be present.
Calcite gets a bit odd here though, even though it accepts "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING/CURRENT ROW", if you open a debugger and check what values we get for the frame clauses you'll see that _isRows is overridden to FALSE by Calcite (meaning it is treating it as RANGE) and the frame boundaries are always set to UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING, irrespective of if you passed CURRENT ROW for the upper limit or not.
So I guess you can say Calcite does implicit RANGE conversion and doesn't even allow a difference in behavior between an upper bound of UNBOUNDED FOLLOWING vs CURRENT ROW if an ORDER BY is not present within the OVER() clause.
Interestingly i had checked PostgreSQL behavior prior to starting this PR and in PostgreSQL they do allow OVER() without ORDER BY to use different upper bounds with ROWS and treats the output differently in that case (will do a rolling aggregation for CURRENT ROW).
Hope that answers your question (though a very long answer π
)
--
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 #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #11577:
URL: https://github.com/apache/pinot/pull/11577#discussion_r1326228139
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -1238,6 +1238,19 @@
"\n"
]
},
+ {
Review Comment:
OVER() or OVER() with only PARTITION BY both only take default frame as "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING/CURRENT ROW". Calcite throws an exception if you modify this to be of RANGE type saying an ORDER BY clause must be present.
Calcite gets a bit odd here though, even though it accepts "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING/CURRENT ROW", if you open a debugger and check what values we get for the frame clauses you'll see that _isRows is overridden to FALSE by Calcite and the frame boundaries are always set to UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING, irrespective of if you passed CURRENT ROW for the upper limit or not.
So I guess you can say Calcite does implicit RANGE conversion and doesn't even allow a difference in behavior between an upper bound of UNBOUNDED FOLLOWING vs CURRENT ROW if an ORDER BY is not present within the OVER() clause.
Interestingly i had checked PostgreSQL behavior prior to starting this PR and in PostgreSQL they do allow OVER() without ORDER BY to use different upper bounds with ROWS and treats the output differently in that case (will do a rolling aggregation for CURRENT ROW).
Hope that answers your question (though a very long answer π
)
--
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 #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #11577:
URL: https://github.com/apache/pinot/pull/11577#discussion_r1326228139
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -1238,6 +1238,19 @@
"\n"
]
},
+ {
Review Comment:
OVER() or OVER() with only PARTITION BY both only take default frame as "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING/CURRENT ROW". Calcite throws an exception if you modify this to be of RANGE type saying an ORDER BY clause must be present.
Calcite gets a bit odd here though, even though it accepts "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING/CURRENT ROW", if you open a debugger and check what values we get for the frame clauses you'll see that _isRows is overridden to FALSE by Calcite and the frame boundaries are always set to UNBOUNDED PRECEDING and UNBOUNDED FOLLOWING, irrespective of if you passed CURRENT ROW for the upper limit or not.
So I guess you can say Calcite does implicit RANGE conversion and doesn't even allow a difference in behavior between an upper bound of UNBOUNDED FOLLOWING vs CURRENT ROW if an ORDER BY is not present within the OVER() clause.
Interestingly i had checked PostgreSQL behavior prior to starting this PR and in PostgreSQL they do allow OVER() without ORDER BY to use different upper bounds with ROWS and treats the output differently in that case (will do a rolling aggregation for CURRENT ROW).
--
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] siddharthteotia commented on a diff in pull request #11577: [multistage] Add ROWS support for aggregation window functions with ORDER BY within OVER
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #11577:
URL: https://github.com/apache/pinot/pull/11577#discussion_r1325405036
##########
pinot-query-planner/src/test/resources/queries/WindowFunctionPlans.json:
##########
@@ -1238,6 +1238,19 @@
"\n"
]
},
+ {
Review Comment:
Do we already have a test for the scenario where if we use `ROWS` frame type for an OVER() clause without `ORDER BY`, it throws syntax error or implicit conversion to `RANGE` frame type ?
--
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