You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:49:50 UTC
[GitHub] [arrow-datafusion] Jimexist opened a new pull request #558: Impl window partition by
Jimexist opened a new pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist commented on pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Jimexist commented on pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#issuecomment-864375798
@Dandandan this is fixed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#issuecomment-860569767
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/558?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 [#558](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a7a499) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/e3e7e293c4482af1475406bf4e922d5c99e7a792?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e3e7e29) will **decrease** coverage by `0.03%`.
> The diff coverage is `81.01%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #558 +/- ##
==========================================
- Coverage 76.12% 76.08% -0.04%
==========================================
Files 156 156
Lines 27074 27121 +47
==========================================
+ Hits 20609 20635 +26
- Misses 6465 6486 +21
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/physical\_plan/window\_functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi93aW5kb3dfZnVuY3Rpb25zLnJz) | `86.42% <ø> (+0.71%)` | :arrow_up: |
| [datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvc3FsL3BsYW5uZXIucnM=) | `84.75% <ø> (ø)` | |
| [datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wbGFubmVyLnJz) | `79.84% <33.33%> (+2.30%)` | :arrow_up: |
| [datafusion/src/physical\_plan/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | `80.00% <70.96%> (+0.90%)` | :arrow_up: |
| [datafusion/src/physical\_plan/windows.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi93aW5kb3dzLnJz) | `82.59% <75.00%> (-3.88%)` | :arrow_down: |
| [...afusion/src/physical\_plan/expressions/nth\_value.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9leHByZXNzaW9ucy9udGhfdmFsdWUucnM=) | `79.41% <75.67%> (-11.07%)` | :arrow_down: |
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.13% <100.00%> (+0.13%)` | :arrow_up: |
| [...fusion/src/physical\_plan/expressions/row\_number.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9leHByZXNzaW9ucy9yb3dfbnVtYmVyLnJz) | `94.28% <100.00%> (+13.03%)` | :arrow_up: |
| [datafusion/src/physical\_plan/hash\_aggregate.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9oYXNoX2FnZ3JlZ2F0ZS5ycw==) | `86.54% <100.00%> (ø)` | |
| [datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvc2NhbGFyLnJz) | `56.19% <100.00%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e3e7e29...4a7a499](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist commented on pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Jimexist commented on pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#issuecomment-864393165
> Looks great again! - 2 comments about tests for being a bit more future proof
fixed, about repartition i'll handle that in #569 but so far i'm seeing regressions in performance
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#issuecomment-864938514
Thanks @Jimexist
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist commented on a change in pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#discussion_r654784583
##########
File path: datafusion/src/execution/context.rs
##########
@@ -1355,6 +1355,90 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn window_partition_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ SUM(c2) OVER (PARTITION BY c2), \
+ COUNT(c2) OVER (PARTITION BY c2), \
+ MAX(c2) OVER (PARTITION BY c2), \
+ MIN(c2) OVER (PARTITION BY c2), \
+ AVG(c2) OVER (PARTITION BY c2) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
Review comment:
fixed
##########
File path: datafusion/src/execution/context.rs
##########
@@ -1355,6 +1355,90 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn window_partition_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ SUM(c2) OVER (PARTITION BY c2), \
+ COUNT(c2) OVER (PARTITION BY c2), \
+ MAX(c2) OVER (PARTITION BY c2), \
+ MIN(c2) OVER (PARTITION BY c2), \
+ AVG(c2) OVER (PARTITION BY c2) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
+
+ let expected = vec![
+ "+----+----+---------+-----------+---------+---------+---------+",
+ "| c1 | c2 | SUM(c2) | COUNT(c2) | MAX(c2) | MIN(c2) | AVG(c2) |",
+ "+----+----+---------+-----------+---------+---------+---------+",
+ "| 0 | 1 | 4 | 4 | 1 | 1 | 1 |",
+ "| 0 | 2 | 8 | 4 | 2 | 2 | 2 |",
+ "| 0 | 3 | 12 | 4 | 3 | 3 | 3 |",
+ "| 0 | 4 | 16 | 4 | 4 | 4 | 4 |",
+ "| 0 | 5 | 20 | 4 | 5 | 5 | 5 |",
+ "+----+----+---------+-----------+---------+---------+---------+",
+ ];
+
+ // window function shall respect ordering
+ assert_batches_eq!(expected, &results);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn window_partition_by_order_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ ROW_NUMBER() OVER (PARTITION BY c2 ORDER BY c1), \
+ FIRST_VALUE(c2 + c1) OVER (PARTITION BY c2 ORDER BY c1), \
+ LAST_VALUE(c2 + c1) OVER (PARTITION BY c2 ORDER BY c1), \
+ NTH_VALUE(c2 + c1, 2) OVER (PARTITION BY c2 ORDER BY c1), \
+ SUM(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ COUNT(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ MAX(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ MIN(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ AVG(c2) OVER (PARTITION BY c2 ORDER BY c1) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#discussion_r654776435
##########
File path: datafusion/src/execution/context.rs
##########
@@ -1355,6 +1355,90 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn window_partition_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ SUM(c2) OVER (PARTITION BY c2), \
+ COUNT(c2) OVER (PARTITION BY c2), \
+ MAX(c2) OVER (PARTITION BY c2), \
+ MIN(c2) OVER (PARTITION BY c2), \
+ AVG(c2) OVER (PARTITION BY c2) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
+
+ let expected = vec![
+ "+----+----+---------+-----------+---------+---------+---------+",
+ "| c1 | c2 | SUM(c2) | COUNT(c2) | MAX(c2) | MIN(c2) | AVG(c2) |",
+ "+----+----+---------+-----------+---------+---------+---------+",
+ "| 0 | 1 | 4 | 4 | 1 | 1 | 1 |",
+ "| 0 | 2 | 8 | 4 | 2 | 2 | 2 |",
+ "| 0 | 3 | 12 | 4 | 3 | 3 | 3 |",
+ "| 0 | 4 | 16 | 4 | 4 | 4 | 4 |",
+ "| 0 | 5 | 20 | 4 | 5 | 5 | 5 |",
+ "+----+----+---------+-----------+---------+---------+---------+",
+ ];
+
+ // window function shall respect ordering
+ assert_batches_eq!(expected, &results);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn window_partition_by_order_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ ROW_NUMBER() OVER (PARTITION BY c2 ORDER BY c1), \
+ FIRST_VALUE(c2 + c1) OVER (PARTITION BY c2 ORDER BY c1), \
+ LAST_VALUE(c2 + c1) OVER (PARTITION BY c2 ORDER BY c1), \
+ NTH_VALUE(c2 + c1, 2) OVER (PARTITION BY c2 ORDER BY c1), \
+ SUM(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ COUNT(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ MAX(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ MIN(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ AVG(c2) OVER (PARTITION BY c2 ORDER BY c1) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
Review comment:
Same here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist commented on a change in pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#discussion_r654755492
##########
File path: datafusion/src/execution/context.rs
##########
@@ -1352,6 +1352,90 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn window_partition_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ SUM(c2) OVER (PARTITION BY c2), \
+ COUNT(c2) OVER (PARTITION BY c2), \
+ MAX(c2) OVER (PARTITION BY c2), \
+ MIN(c2) OVER (PARTITION BY c2), \
+ AVG(c2) OVER (PARTITION BY c2) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
+
+ let expected = vec![
+ "+----+----+---------+-----------+---------+---------+---------+",
+ "| c1 | c2 | SUM(c2) | COUNT(c2) | MAX(c2) | MIN(c2) | AVG(c2) |",
+ "+----+----+---------+-----------+---------+---------+---------+",
+ "| 0 | 1 | 4 | 4 | 1 | 1 | 1 |",
+ "| 0 | 2 | 8 | 4 | 2 | 2 | 2 |",
+ "| 0 | 3 | 12 | 4 | 3 | 3 | 3 |",
+ "| 0 | 4 | 16 | 4 | 4 | 4 | 4 |",
+ "| 0 | 5 | 20 | 4 | 5 | 5 | 5 |",
+ "+----+----+---------+-----------+---------+---------+---------+",
+ ];
+
+ // window function shall respect ordering
+ assert_batches_eq!(expected, &results);
+ Ok(())
+ }
+
+ #[tokio::test]
+ async fn window_partition_by_order_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ ROW_NUMBER() OVER (PARTITION BY c2 ORDER BY c1), \
+ FIRST_VALUE(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ LAST_VALUE(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ NTH_VALUE(c2, 2) OVER (PARTITION BY c2 ORDER BY c1), \
+ SUM(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ COUNT(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ MAX(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ MIN(c2) OVER (PARTITION BY c2 ORDER BY c1), \
+ AVG(c2) OVER (PARTITION BY c2 ORDER BY c1) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
+
+ let expected = vec![
+ "+----+----+--------------+-----------------+----------------+------------------------+---------+-----------+---------+---------+---------+",
+ "| c1 | c2 | ROW_NUMBER() | FIRST_VALUE(c2) | LAST_VALUE(c2) | NTH_VALUE(c2,Int64(2)) | SUM(c2) | COUNT(c2) | MAX(c2) | MIN(c2) | AVG(c2) |",
+ "+----+----+--------------+-----------------+----------------+------------------------+---------+-----------+---------+---------+---------+",
Review comment:
i updated the function parameters so now they are different
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#discussion_r654776646
##########
File path: datafusion/src/physical_plan/expressions/nth_value.rs
##########
@@ -135,8 +135,12 @@ impl BuiltInWindowFunctionExpr for NthValue {
NthValueKind::Last => (num_rows as usize) - 1,
NthValueKind::Nth(n) => (n as usize) - 1,
};
- let value = ScalarValue::try_from_array(value, index)?;
- Ok(value.to_array_of_size(num_rows))
+ Ok(if index >= num_rows {
+ new_null_array(value.data_type(), num_rows)
+ } else {
+ let value = ScalarValue::try_from_array(value, index)?;
+ value.to_array_of_size(num_rows)
Review comment:
Yes - that should probably already be quite an improvement :+1:
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter commented on pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#issuecomment-860569767
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/558?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 [#558](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7cb72ec) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/d3828541a61b5681b93590a47e22d63715949136?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d382854) will **decrease** coverage by `0.03%`.
> The diff coverage is `81.27%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #558 +/- ##
==========================================
- Coverage 76.09% 76.05% -0.04%
==========================================
Files 156 156
Lines 27047 27098 +51
==========================================
+ Hits 20581 20609 +28
- Misses 6466 6489 +23
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/physical\_plan/window\_functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi93aW5kb3dfZnVuY3Rpb25zLnJz) | `86.42% <ø> (+0.71%)` | :arrow_up: |
| [datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvc3FsL3BsYW5uZXIucnM=) | `84.75% <ø> (-0.11%)` | :arrow_down: |
| [datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wbGFubmVyLnJz) | `79.84% <33.33%> (+2.30%)` | :arrow_up: |
| [datafusion/src/physical\_plan/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | `80.00% <70.96%> (+0.90%)` | :arrow_up: |
| [datafusion/src/physical\_plan/windows.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi93aW5kb3dzLnJz) | `82.59% <75.00%> (-3.88%)` | :arrow_down: |
| [...afusion/src/physical\_plan/expressions/nth\_value.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9leHByZXNzaW9ucy9udGhfdmFsdWUucnM=) | `79.41% <75.67%> (-11.07%)` | :arrow_down: |
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.13% <100.00%> (+0.13%)` | :arrow_up: |
| [...fusion/src/physical\_plan/expressions/row\_number.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9leHByZXNzaW9ucy9yb3dfbnVtYmVyLnJz) | `94.28% <100.00%> (+13.03%)` | :arrow_up: |
| [datafusion/src/physical\_plan/hash\_aggregate.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9oYXNoX2FnZ3JlZ2F0ZS5ycw==) | `86.54% <100.00%> (ø)` | |
| [datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvc2NhbGFyLnJz) | `56.19% <100.00%> (ø)` | |
| ... and [9 more](https://codecov.io/gh/apache/arrow-datafusion/pull/558/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d382854...7cb72ec](https://codecov.io/gh/apache/arrow-datafusion/pull/558?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#issuecomment-864938514
Thanks @Jimexist
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #558: Implement window functions with `partition_by` clause
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #558:
URL: https://github.com/apache/arrow-datafusion/pull/558#discussion_r654776385
##########
File path: datafusion/src/execution/context.rs
##########
@@ -1355,6 +1355,90 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn window_partition_by() -> Result<()> {
+ let results = execute(
+ "SELECT \
+ c1, \
+ c2, \
+ SUM(c2) OVER (PARTITION BY c2), \
+ COUNT(c2) OVER (PARTITION BY c2), \
+ MAX(c2) OVER (PARTITION BY c2), \
+ MIN(c2) OVER (PARTITION BY c2), \
+ AVG(c2) OVER (PARTITION BY c2) \
+ FROM test \
+ ORDER BY c1, c2 \
+ LIMIT 5",
+ 4,
+ )
+ .await?;
+ dbg!(results.len());
+ // result in one batch, although e.g. having 2 batches do not change
+ // result semantics, having a len=1 assertion upfront keeps surprises
+ // at bay
+ assert_eq!(results.len(), 1);
Review comment:
I think this shouldn't be tested (with the same reasoning of the comment).
For example, if we would split the data in different partitions based on hashing the partition by expression, we are going to emit multiple batches.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org