You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mithuncy (via GitHub)" <gi...@apache.org> on 2026/07/09 08:56:42 UTC
[PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
mithuncy opened a new pull request, #23416:
URL: https://github.com/apache/datafusion/pull/23416
## Which issue does this PR close?
- Closes #23289.
Related:
- EPIC: #22395
- Same pattern for aggregations: #23239
- Stacked on (and requires): #23184 — only the last commit here is this PR; the first commit is that PR's base. Will rebase and mark ready once it merges.
- Sibling rollout for PartitionedTopK on the same base: #23355. Note that PR currently also opts the two window execs into range satisfaction as part of its PartitionedTopK work; this PR is the dedicated #23289 change with the window-specific test suite the issue asks for, and scopes the opt-in to the keyed branch only. Happy to coordinate / rebase whichever lands second (the slt test numbering in `range_partitioning.slt` will need reconciling too).
- General `Range` satisfaction (future work): #23266
## Rationale for this change
`WindowAggExec` and `BoundedWindowAggExec` require
`Distribution::KeyPartitioned(partition_keys)`: all rows with the same
`PARTITION BY` key must be in the same partition. An input that is already
range partitioned on a compatible key (e.g. `Range([a])` for
`PARTITION BY a`) provides exactly that colocation, but only aggregates
currently opt in to range satisfaction. As a result, an unnecessary
`RepartitionExec: partitioning=Hash(...)` is inserted in front of windows
even when the input's range partitioning already satisfies the
requirement.
This PR is stacked on #23184, which introduces
`InputDistributionRequirements` and routes distribution satisfaction
through an operator-declared policy (replacing the optimizer-side private
gate from #23239). On that base, enabling windows is a declaration-level
change on the operators themselves.
## What changes are included in this PR?
Core change (two files, no optimizer changes):
- `datafusion/physical-plan/src/windows/window_agg_exec.rs` and
`datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs`:
when `partition_keys()` is non-empty, the
`input_distribution_requirements()` declaration now appends
`.allow_range_satisfaction_for_key_partitioning()`, mirroring
`AggregateExec` on the base branch. The `ensure_distribution` and
`SanityCheckPlan` machinery honor the declared policy with no
operator-specific code.
Notes on scope:
- Windows with no `PARTITION BY`, or with
`datafusion.optimizer.repartition_windows = false`, have empty
`partition_keys()` and keep requiring `SinglePartition`; the opt-in is
not applied to them.
- Exact satisfaction (`Range([a])` for `PARTITION BY a`), subset
satisfaction (`Range([a])` for `PARTITION BY a, b` when enabled), and
hash fallback for incompatible keys all come from the base branch's
satisfaction machinery, unchanged.
- The per-operator opt-in is a rollout marker: once `Range` satisfies
`KeyPartitioned` generally (#23266), these opt-ins get deleted.
- Joins/co-partitioning are entirely the base PR's (#23184) concern; this
PR only touches single-input window operators.
## Are these changes tested?
Yes.
- SLT (`range_partitioning.slt`, TESTs 19–26, appended after the base
branch's join tests), each `EXPLAIN` paired with a result query where
its aggregate-section precedent has one:
- exact reuse: `PARTITION BY range_key` over `Range([range_key])` plans
with no repartition (window over a per-partition sort over the range
source)
- a non-streamable frame (`ROWS BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING`) planning as `WindowAggExec` — covers the
non-bounded operator
- incompatible key: `PARTITION BY non_range_key` still hash repartitions
- subset satisfaction enabled: `PARTITION BY range_key, non_range_key`
avoids the hash repartition
- subset threshold not met: the same query hash repartitions
- `preserve_file_partitions` met: range preserved even when
`target_partitions` exceeds the input partition count
- subset threshold not met with higher `target_partitions`:
repartitions to increase parallelism
- no `PARTITION BY`: single-partition plan unchanged
- Plan-shape tests in `enforce_distribution.rs`: exact-positive
(no hash repartition over compatible range input) and negative
(incompatible key still rehashes), mirroring the aggregate tests from
#23239.
- Sanity checker tests: a window over a compatible range-partitioned
input passes `SanityCheckPlan`; an incompatible one is rejected. These
reuse the file-local `range_partitioned_exec` fixture the base branch
added for its join sanity test.
- Test infra: a `bounded_window_exec_with_can_repartition` builder
variant (the pre-existing builder hardcodes `can_repartition = false`,
under which window distribution tests pass vacuously).
- Full local run: fmt, `./dev/rust_lint.sh`, the extended workspace test
suite (`--features avro,json,backtrace,extended_tests,
recursive_protection,parquet_encryption`), and the `range_partitioning`
slt file all pass on this branch.
## Are there any user-facing changes?
No API changes. Plans over range-partitioned sources may now avoid a hash
repartition in front of window operators (a performance improvement); the
behavior is controlled by the existing
`datafusion.optimizer.subset_repartition_threshold`,
`datafusion.optimizer.preserve_file_partitions`, and
`datafusion.execution.target_partitions` settings.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4971874582
this is now good thank you 👍
cc: @gabotechs
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "mithuncy (via GitHub)" <gi...@apache.org>.
mithuncy commented on code in PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#discussion_r3564613658
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -325,12 +325,15 @@ impl ExecutionPlan for BoundedWindowAggExec {
}
fn input_distribution_requirements(&self) -> crate::InputDistributionRequirements {
- crate::InputDistributionRequirements::new(if self.partition_keys().is_empty() {
+ if self.partition_keys().is_empty() {
debug!("No partition defined for BoundedWindowAggExec!!!");
- vec![Distribution::SinglePartition]
+ crate::InputDistributionRequirements::new(vec![Distribution::SinglePartition])
Review Comment:
Done — imported InputDistributionRequirements in both window execs
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4956032357
This looks good, +1 to @gene-bordegaray's suggestion about tests.
@gene-bordegaray whenever you give me the greenlight I'll +1 and merge 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#discussion_r3564120959
##########
datafusion/sqllogictest/test_files/range_partitioning.slt:
##########
@@ -682,5 +682,344 @@ ORDER BY range_key, value;
35 350
35 350
+statement ok
+set datafusion.execution.target_partitions = 4;
+
+statement ok
+set datafusion.optimizer.subset_repartition_threshold = 4;
+
+statement ok
+set datafusion.optimizer.preserve_file_partitions = 0;
+
+
+##########
+# TEST 19: Window on Range Partition Column
+# Range([range_key]) colocates equal range_key values, so
+# PARTITION BY range_key is satisfied without a hash repartition.
+##########
+
+query TT
+EXPLAIN SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value) FROM range_partitioned;
+----
+physical_plan
+01)ProjectionExec: expr=[range_key@0 as range_key, sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@2 as sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW]
+02)--BoundedWindowAggExec: wdw=[sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Field { "sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable Int64 }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
+03)----SortExec: expr=[range_key@0 ASC NULLS LAST, value@1 ASC NULLS LAST], preserve_partitioning=[true]
+04)------DataSourceExec: file_groups=<slt:ignore>, projection=[range_key, value], output_partitioning=Range([range_key@0 ASC], [(10), (20), (30)], 4), file_type=csv, has_header=false
+
+query II
+SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value) FROM range_partitioned ORDER BY range_key;
+----
+1 10
+5 50
+10 100
+15 150
+20 200
+25 250
+30 300
+35 350
+
+
+##########
+# TEST 20: Unbounded-Frame Window on Range Partition Column
+# The unbounded frame makes DataFusion use WindowAggExec instead of
+# BoundedWindowAggExec, which likewise reuses Range partitioning without a
+# hash repartition.
+##########
+
+query TT
+EXPLAIN SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM range_partitioned;
+----
+physical_plan
+01)ProjectionExec: expr=[range_key@0 as range_key, sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@2 as sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]
+02)--WindowAggExec: wdw=[sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Ok(Field { name: "sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING", data_type: Int64, nullable: true }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)), end_bound: Following(UInt64(NULL)), is_causal: false }]
+03)----SortExec: expr=[range_key@0 ASC NULLS LAST, value@1 ASC NULLS LAST], preserve_partitioning=[true]
+04)------DataSourceExec: file_groups=<slt:ignore>, projection=[range_key, value], output_partitioning=Range([range_key@0 ASC], [(10), (20), (30)], 4), file_type=csv, has_header=false
+
+query II
+SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM range_partitioned ORDER BY range_key;
+----
+1 10
+5 50
+10 100
+15 150
+20 200
+25 250
+30 300
+35 350
+
+
+##########
+# TEST 21: Window on Non-Range Column Rehashes
+# Range([range_key]) does not colocate non_range_key values, so
+# PARTITION BY non_range_key still requires a hash repartition.
+##########
+
+query TT
+EXPLAIN SELECT non_range_key, SUM(value) OVER (PARTITION BY non_range_key ORDER BY value) FROM range_partitioned;
+----
+physical_plan
+01)ProjectionExec: expr=[non_range_key@0 as non_range_key, sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@2 as sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW]
+02)--BoundedWindowAggExec: wdw=[sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Field { "sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable Int64 }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
Review Comment:
what do you think about doing one negative case for `WindowAggExec`
##########
datafusion/sqllogictest/test_files/range_partitioning.slt:
##########
@@ -682,5 +682,344 @@ ORDER BY range_key, value;
35 350
35 350
+statement ok
+set datafusion.execution.target_partitions = 4;
+
+statement ok
+set datafusion.optimizer.subset_repartition_threshold = 4;
+
+statement ok
+set datafusion.optimizer.preserve_file_partitions = 0;
+
+
+##########
Review Comment:
I think the coverage is great here, I do want to avoid this file from getting too noisy with checking file preserving and subset satisfaction for each operator though. What if we did something like test:
1. Bounded window accepts exact Range(a) (test 19)
2. General window accepts exact Range(a) (test 20)
4. Range(a) does not satisfy PARTITION BY b, and a hash is added for bounded and unbounded (test 21 and new test)
5. Safe range subset, Range(a) satisfies PARTITION BY (a, b) when subset is enabled (test 22)
6. Subset disabled, the same query gets a hash exchange (test 24)
7. No PARTITION BY, gets one logical window (test 28)
Then we should add a matrix test for these configurations as a unit / integration test in `enforce_distribution.rs` to ensure we are inserting repartitions as expeced according to these. I think this is fitting as those configs are tightly coupled with that file. I know @2010YOUY01 had some good guidance on test, would love your two cents here too 👍
##########
datafusion/sqllogictest/test_files/range_partitioning.slt:
##########
@@ -682,5 +682,344 @@ ORDER BY range_key, value;
35 350
35 350
+statement ok
+set datafusion.execution.target_partitions = 4;
+
+statement ok
+set datafusion.optimizer.subset_repartition_threshold = 4;
+
+statement ok
+set datafusion.optimizer.preserve_file_partitions = 0;
+
+
+##########
Review Comment:
this is a non-blocker for this PR though, something I am taking note of
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs merged PR #23416:
URL: https://github.com/apache/datafusion/pull/23416
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "mithuncy (via GitHub)" <gi...@apache.org>.
mithuncy commented on code in PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#discussion_r3564614676
##########
datafusion/sqllogictest/test_files/range_partitioning.slt:
##########
@@ -682,5 +682,344 @@ ORDER BY range_key, value;
35 350
35 350
+statement ok
+set datafusion.execution.target_partitions = 4;
+
+statement ok
+set datafusion.optimizer.subset_repartition_threshold = 4;
+
+statement ok
+set datafusion.optimizer.preserve_file_partitions = 0;
+
+
+##########
+# TEST 19: Window on Range Partition Column
+# Range([range_key]) colocates equal range_key values, so
+# PARTITION BY range_key is satisfied without a hash repartition.
+##########
+
+query TT
+EXPLAIN SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value) FROM range_partitioned;
+----
+physical_plan
+01)ProjectionExec: expr=[range_key@0 as range_key, sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@2 as sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW]
+02)--BoundedWindowAggExec: wdw=[sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Field { "sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable Int64 }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
+03)----SortExec: expr=[range_key@0 ASC NULLS LAST, value@1 ASC NULLS LAST], preserve_partitioning=[true]
+04)------DataSourceExec: file_groups=<slt:ignore>, projection=[range_key, value], output_partitioning=Range([range_key@0 ASC], [(10), (20), (30)], 4), file_type=csv, has_header=false
+
+query II
+SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value) FROM range_partitioned ORDER BY range_key;
+----
+1 10
+5 50
+10 100
+15 150
+20 200
+25 250
+30 300
+35 350
+
+
+##########
+# TEST 20: Unbounded-Frame Window on Range Partition Column
+# The unbounded frame makes DataFusion use WindowAggExec instead of
+# BoundedWindowAggExec, which likewise reuses Range partitioning without a
+# hash repartition.
+##########
+
+query TT
+EXPLAIN SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM range_partitioned;
+----
+physical_plan
+01)ProjectionExec: expr=[range_key@0 as range_key, sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@2 as sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]
+02)--WindowAggExec: wdw=[sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Ok(Field { name: "sum(range_partitioned.value) PARTITION BY [range_partitioned.range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING", data_type: Int64, nullable: true }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)), end_bound: Following(UInt64(NULL)), is_causal: false }]
+03)----SortExec: expr=[range_key@0 ASC NULLS LAST, value@1 ASC NULLS LAST], preserve_partitioning=[true]
+04)------DataSourceExec: file_groups=<slt:ignore>, projection=[range_key, value], output_partitioning=Range([range_key@0 ASC], [(10), (20), (30)], 4), file_type=csv, has_header=false
+
+query II
+SELECT range_key, SUM(value) OVER (PARTITION BY range_key ORDER BY value ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) FROM range_partitioned ORDER BY range_key;
+----
+1 10
+5 50
+10 100
+15 150
+20 200
+25 250
+30 300
+35 350
+
+
+##########
+# TEST 21: Window on Non-Range Column Rehashes
+# Range([range_key]) does not colocate non_range_key values, so
+# PARTITION BY non_range_key still requires a hash repartition.
+##########
+
+query TT
+EXPLAIN SELECT non_range_key, SUM(value) OVER (PARTITION BY non_range_key ORDER BY value) FROM range_partitioned;
+----
+physical_plan
+01)ProjectionExec: expr=[non_range_key@0 as non_range_key, sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@2 as sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW]
+02)--BoundedWindowAggExec: wdw=[sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Field { "sum(range_partitioned.value) PARTITION BY [range_partitioned.non_range_key] ORDER BY [range_partitioned.value ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable Int64 }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
Review Comment:
added as TEST 22 right after the bounded negative
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4923954726
This is in my queue, just waiting for #23184 to land 🙇
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4923477195
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v54.0.0 (current)
Built [ 101.901s] (current)
Parsing datafusion v54.0.0 (current)
Parsed [ 0.034s] (current)
Building datafusion v54.0.0 (baseline)
Built [ 101.040s] (baseline)
Parsing datafusion v54.0.0 (baseline)
Parsed [ 0.036s] (baseline)
Checking datafusion v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.625s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 205.594s] datafusion
Building datafusion-datasource v54.0.0 (current)
Built [ 37.461s] (current)
Parsing datafusion-datasource v54.0.0 (current)
Parsed [ 0.032s] (current)
Building datafusion-datasource v54.0.0 (baseline)
Built [ 37.587s] (baseline)
Parsing datafusion-datasource v54.0.0 (baseline)
Parsed [ 0.033s] (baseline)
Checking datafusion-datasource v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.245s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 76.676s] datafusion-datasource
Building datafusion-physical-expr v54.0.0 (current)
Built [ 28.629s] (current)
Parsing datafusion-physical-expr v54.0.0 (current)
Parsed [ 0.045s] (current)
Building datafusion-physical-expr v54.0.0 (baseline)
Built [ 28.958s] (baseline)
Parsing datafusion-physical-expr v54.0.0 (baseline)
Parsed [ 0.047s] (baseline)
Checking datafusion-physical-expr v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.340s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/inherent_method_missing.ron
Failed in:
RangePartitioning::compatible_with, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/40bec6fa31fb8209d998577a194058efbe5f43b7/datafusion/physical-expr/src/partitioning.rs:253
Partitioning::compatible_with, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/40bec6fa31fb8209d998577a194058efbe5f43b7/datafusion/physical-expr/src/partitioning.rs:416
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 58.786s] datafusion-physical-expr
Building datafusion-physical-optimizer v54.0.0 (current)
Built [ 39.043s] (current)
Parsing datafusion-physical-optimizer v54.0.0 (current)
Parsed [ 0.022s] (current)
Building datafusion-physical-optimizer v54.0.0 (baseline)
Built [ 38.762s] (baseline)
Parsing datafusion-physical-optimizer v54.0.0 (baseline)
Parsed [ 0.022s] (baseline)
Checking datafusion-physical-optimizer v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.114s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 78.920s] datafusion-physical-optimizer
Building datafusion-physical-plan v54.0.0 (current)
Built [ 36.486s] (current)
Parsing datafusion-physical-plan v54.0.0 (current)
Parsed [ 0.140s] (current)
Building datafusion-physical-plan v54.0.0 (baseline)
Built [ 36.906s] (baseline)
Parsing datafusion-physical-plan v54.0.0 (baseline)
Parsed [ 0.136s] (baseline)
Checking datafusion-physical-plan v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.642s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure trait_method_marked_deprecated: trait method #[deprecated] added ---
Description:
A trait method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/trait_method_marked_deprecated.ron
Failed in:
method required_input_distribution in trait datafusion_physical_plan::execution_plan::ExecutionPlan in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/execution_plan.rs:98
method required_input_distribution in trait datafusion_physical_plan::ExecutionPlan in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/execution_plan.rs:98
Summary semver requires new minor version: 0 major and 1 minor checks failed
Finished [ 75.401s] datafusion-physical-plan
Building datafusion-sqllogictest v54.0.0 (current)
Built [ 176.834s] (current)
Parsing datafusion-sqllogictest v54.0.0 (current)
Parsed [ 0.020s] (current)
Building datafusion-sqllogictest v54.0.0 (baseline)
Built [ 176.343s] (baseline)
Parsing datafusion-sqllogictest v54.0.0 (baseline)
Parsed [ 0.023s] (baseline)
Checking datafusion-sqllogictest v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.088s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 355.910s] datafusion-sqllogictest
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#discussion_r3564295834
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -325,12 +325,15 @@ impl ExecutionPlan for BoundedWindowAggExec {
}
fn input_distribution_requirements(&self) -> crate::InputDistributionRequirements {
- crate::InputDistributionRequirements::new(if self.partition_keys().is_empty() {
+ if self.partition_keys().is_empty() {
debug!("No partition defined for BoundedWindowAggExec!!!");
- vec![Distribution::SinglePartition]
+ crate::InputDistributionRequirements::new(vec![Distribution::SinglePartition])
Review Comment:
nit: can we also import `InputDistributionRequirements`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4970977131
@mithuncy some work seems to be blocked on this. Do you have an idea about when you can update remaining tests 😄
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "mithuncy (via GitHub)" <gi...@apache.org>.
mithuncy commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4971893280
> this is now good thank you 👍
@gene-bordegaray Thanks a lot. follow up pr https://github.com/apache/datafusion/issues/23572
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "mithuncy (via GitHub)" <gi...@apache.org>.
mithuncy commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4971447948
> @mithuncy some work seems to be blocked on this. Do you have an idea about when you can update remaining tests
Sorry for the delay — on it now, will push within the hour.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#issuecomment-4972722021
Thanks @mithuncy for the PR, and @gene-bordegaray for the review! merging this one 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "mithuncy (via GitHub)" <gi...@apache.org>.
mithuncy commented on code in PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#discussion_r3564616991
##########
datafusion/sqllogictest/test_files/range_partitioning.slt:
##########
@@ -682,5 +682,344 @@ ORDER BY range_key, value;
35 350
35 350
+statement ok
+set datafusion.execution.target_partitions = 4;
+
+statement ok
+set datafusion.optimizer.subset_repartition_threshold = 4;
+
+statement ok
+set datafusion.optimizer.preserve_file_partitions = 0;
+
+
+##########
Review Comment:
Agreed on keeping this file lean. Will do either in this PR or as an immediate follow-up — whichever you and @2010YOUY01 prefer once he's had a look.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: allow Partitioning::Range to satisfy window Distribution::KeyPartitioned requirements [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #23416:
URL: https://github.com/apache/datafusion/pull/23416#discussion_r3564700497
##########
datafusion/sqllogictest/test_files/range_partitioning.slt:
##########
@@ -682,5 +682,344 @@ ORDER BY range_key, value;
35 350
35 350
+statement ok
+set datafusion.execution.target_partitions = 4;
+
+statement ok
+set datafusion.optimizer.subset_repartition_threshold = 4;
+
+statement ok
+set datafusion.optimizer.preserve_file_partitions = 0;
+
+
+##########
Review Comment:
I think lets keep fil lean in this PR and make follow up for the `enforce_distribution.rs` matrix tests 😄 . Lmk thoughts @2010YOUY01
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org