Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-16 Thread via GitHub 


gene-bordegaray commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3252707926


##
datafusion/physical-plan/src/repartition/mod.rs:
##
@@ -600,6 +600,11 @@ impl BatchPartitioner {
 num_input_partitions,
 ))
 }
+Partitioning::Expr(_) => {
+not_impl_err!(
+"Expression partitioning is not supported by 
RepartitionExec"
+)
+}

Review Comment:
   I continued conversation at #21992 ! I didn't fully consider this so thank 
you 😄 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


stuhood commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3250968025


##
datafusion/physical-plan/src/repartition/mod.rs:
##
@@ -600,6 +600,11 @@ impl BatchPartitioner {
 num_input_partitions,
 ))
 }
+Partitioning::Expr(_) => {
+not_impl_err!(
+"Expression partitioning is not supported by 
RepartitionExec"
+)
+}

Review Comment:
   > My intent wasn't for `ExprPartitioning` to be efficient execution format 
for physically repartitioning rows. I was thinking of this as partitioning for 
sources/plans that already have known partitioning and declare it to preserve 
in the plan to unlock optimizations.
   
   That works for the first join, but not for followup joins. For example:
   
   If you have a 3 table join, the first join will be able to use an equality 
match on range partitioning to say: no re-partitioning needed at all because 
the two tables are partitioned the same way! Great.
   
   But its very likely that the second join _does_ need to re-partition _one_ 
of its inputs (assuming different join keys between the two joins): the output 
of join one needs to be re-partitioned to match the third table. Now, 
technically you can just repartition both sides (i.e. switch to hash or 
something). But if you instead re-partition to _match_ the second table, then 
you might be able to significantly cut down on data movement.
   
   
   
   So, yes: I think that it is important to be able to efficiently re-partition 
by this strategy. If we don't have concrete use-cases for generic expression 
partitioning, then it would not be my first choice 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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


stuhood commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3250968025


##
datafusion/physical-plan/src/repartition/mod.rs:
##
@@ -600,6 +600,11 @@ impl BatchPartitioner {
 num_input_partitions,
 ))
 }
+Partitioning::Expr(_) => {
+not_impl_err!(
+"Expression partitioning is not supported by 
RepartitionExec"
+)
+}

Review Comment:
   > My intent wasn't for `ExprPartitioning` to be efficient execution format 
for physically repartitioning rows. I was thinking of this as partitioning for 
sources/plans that already have known partitioning and declare it to preserve 
in the plan to unlock optimizations.
   
   That works for the first join, but not for followup joins. For example:
   
   If you have a 3 table join, the first join will be able to use an equality 
match on range partitioning to say: no re-partitioning needed at all because 
the two tables are partitioned the same way! Great.
   
   But its very likely that the second join _does_ need to re-partition _one_ 
of its inputs (assuming different join keys between the two joins): the output 
of join one needs to be re-partitioned to match the third table. Now, 
technically you can just repartition both sides (i.e. switch to hash or 
something). But if you instead re-partition to _match_ the third table, then 
you might be able to significantly cut down on data movement.
   
   
   
   So, yes: I think that it is important to be able to efficiently re-partition 
by this strategy. If we don't have concrete use-cases for generic expression 
partitioning, then it would not be my first choice 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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


gene-bordegaray commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3250129734


##
datafusion/physical-plan/src/repartition/mod.rs:
##
@@ -600,6 +600,11 @@ impl BatchPartitioner {
 num_input_partitions,
 ))
 }
+Partitioning::Expr(_) => {
+not_impl_err!(
+"Expression partitioning is not supported by 
RepartitionExec"
+)
+}

Review Comment:
   My intent wasn't for `ExprPartitioning` to be efficient execution format for 
physically repartitioning rows. I was thinking of this as partitioning for 
sources/plans that already have known partitioning and declare it to preserve 
in the plan to unlock optimizations.
   
   In follow-ups:
   - add explicit compatibility/satisfaction APIs around this metadata we can 
ask structured questions without doing row-wise linear scans. This would 
eliminate uneeded repartitions in cases where different partitioning types 
satisfy one another. 
   - keep hash repartitioning as the preferred general execution path when 
DataFusion needs to repartition arbitrary input, unless we later add a more 
specialized repartitioning strategy.
   
   Let me know thoughts on that 👍 
   



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


gene-bordegaray commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3249849428


##
datafusion/physical-expr/src/partitioning.rs:
##
@@ -133,13 +135,94 @@ impl Display for Partitioning {
 .join(", ");
 write!(f, "Hash([{phy_exprs_str}], {size})")
 }
+Partitioning::Expr(expr) => write!(f, "{expr}"),
 Partitioning::UnknownPartitioning(size) => {
 write!(f, "UnknownPartitioning({size})")
 }
 }
 }
 }
 
+/// Physical expression partitioning.
+///
+/// Partition `i` contains rows where `partition_exprs[i]` evaluates to true.
+/// The source declaring partitioning is responsible for ensuring that, for 
every
+/// row emitted, exactly one partition expression evaluates to true and that 
row
+/// is emitted by the corresponding partition. The expressions do not need to
+/// cover values that the plan cannot emit.
+///
+/// For example, a scan that can only emit rows for `2022` can declare two date
+/// partitions as:
+///
+/// ```text
+/// partition_exprs[0] = date >= 2022-01-01 AND date < 2022-07-01
+/// partition_exprs[1] = date >= 2022-07-01 AND date < 2023-01-01
+/// ```
+///
+/// This is valid even though values outside `2022` are not covered, as long as
+/// the source does not emit rows outside those ranges. It would not be valid
+/// for this plan to emit a row from `partition[i]` whose date is not within
+/// `partition_exprs[i]`, or to emit a row whose date matches multiple
+/// partition expressions.
+///
+/// More complex partitioning can be represented using normal expression
+/// composition. For example, one partition in a date and city range can be
+/// represented as `date >= 2021-01-01 AND date < 2022-07-01 AND city < 
'Boston'`.
+///
+/// NOTE: Optimizer and execution behavior for this partitioning is 
intentionally
+/// not implemented and will be introduced incrementally.
+#[derive(Debug, Clone)]
+pub struct ExprPartitioning {

Review Comment:
   The reason is that we aim to be more flexible here. This can support Range 
partitioning but also extens beyond that to any physical expr the source wants 
to provide. I just gave range in the description as one concrete example of how 
this could be used.
   
   Someone could partition using this scheme on something like city column 
where:
   
   ```text
   partition 1 -> city = "New York"
   partition 2 -> city = "London"
   ```
   
   and so on.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


stuhood commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3249785424


##
datafusion/physical-plan/src/repartition/mod.rs:
##
@@ -600,6 +600,11 @@ impl BatchPartitioner {
 num_input_partitions,
 ))
 }
+Partitioning::Expr(_) => {
+not_impl_err!(
+"Expression partitioning is not supported by 
RepartitionExec"
+)
+}

Review Comment:
   So, it's worth discussing this in more detail I think.
   
   `Expr` partitioning is much, much more general than `Range` partitioning.
   
   In `Range` partitioning, deciding which partition a row maps to involves 
either a binary search or sorted map lookup. But in `Expr` partitioning, it 
will always be a linear scan through the expressions, unless the consumer has 
reverse-engineered the fact that it is actually Range partitioning under the 
hood.
   
   So this operator will be much more expensive than it might be otherwise.
   
   What is the reasoning around using expressions here, and not literally 
ranges?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


stuhood commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3249785424


##
datafusion/physical-plan/src/repartition/mod.rs:
##
@@ -600,6 +600,11 @@ impl BatchPartitioner {
 num_input_partitions,
 ))
 }
+Partitioning::Expr(_) => {
+not_impl_err!(
+"Expression partitioning is not supported by 
RepartitionExec"
+)
+}

Review Comment:
   So, it's worth discussing this in more detail I think.
   
   `Expr` partitioning is much, much more general than `Range` partitioning.
   
   In `Range` partitioning, deciding which partition a row maps to involves 
either a binary search or sorted map lookup. But in `Expr` partitioning, it 
will always be a linear scan through the expressions, unless the consumer has 
reverse-engineered the fact that it is actually `Range` partitioning under the 
hood.
   
   So this operator will be much more expensive than it might be otherwise.
   
   What is the reasoning around using expressions here, and not literally 
ranges?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


github-actions[bot] commented on PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#issuecomment-4461773086

   
   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
   
   ```
Cloning apache/main
   Building datafusion-ffi v53.1.0 (current)
  Built [  57.638s] (current)
Parsing datafusion-ffi v53.1.0 (current)
 Parsed [   0.059s] (current)
   Building datafusion-ffi v53.1.0 (baseline)
  Built [  57.775s] (baseline)
Parsing datafusion-ffi v53.1.0 (baseline)
 Parsed [   0.060s] (baseline)
   Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [   0.232s] 222 checks: 222 pass, 30 skip
Summary no semver update required
   Finished [ 117.281s] datafusion-ffi
   Building datafusion-physical-expr v53.1.0 (current)
  Built [  24.334s] (current)
Parsing datafusion-physical-expr v53.1.0 (current)
 Parsed [   0.043s] (current)
   Building datafusion-physical-expr v53.1.0 (baseline)
  Built [  24.338s] (baseline)
Parsing datafusion-physical-expr v53.1.0 (baseline)
 Parsed [   0.044s] (baseline)
   Checking datafusion-physical-expr v53.1.0 -> v53.1.0 (no change; assume 
patch)
Checked [   0.319s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
   
   --- failure enum_variant_added: enum variant added on exhaustive enum ---
   
   Description:
   A publicly-visible enum without #[non_exhaustive] has a new variant.
   ref: 
https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
  impl: 
https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/enum_variant_added.ron
   
   Failed in:
 variant Partitioning:Expr in 
/home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/partitioning.rs:121
   
Summary semver requires new major version: 1 major and 0 minor checks 
failed
   Finished [  50.073s] datafusion-physical-expr
   Building datafusion-physical-plan v53.1.0 (current)
  Built [  32.255s] (current)
Parsing datafusion-physical-plan v53.1.0 (current)
 Parsed [   0.122s] (current)
   Building datafusion-physical-plan v53.1.0 (baseline)
  Built [  32.175s] (baseline)
Parsing datafusion-physical-plan v53.1.0 (baseline)
 Parsed [   0.122s] (baseline)
   Checking datafusion-physical-plan v53.1.0 -> v53.1.0 (no change; assume 
patch)
Checked [   0.550s] 222 checks: 222 pass, 30 skip
Summary no semver update required
   Finished [  66.410s] datafusion-physical-plan
   Building datafusion-proto v53.1.0 (current)
  Built [  52.788s] (current)
Parsing datafusion-proto v53.1.0 (current)
 Parsed [   0.139s] (current)
   Building datafusion-proto v53.1.0 (baseline)
  Built [  52.704s] (baseline)
Parsing datafusion-proto v53.1.0 (baseline)
 Parsed [   0.134s] (baseline)
   Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [   1.660s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
   
   --- failure enum_variant_added: enum variant added on exhaustive enum ---
   
   Description:
   A publicly-visible enum without #[non_exhaustive] has a new variant.
   ref: 
https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
  impl: 
https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/enum_variant_added.ron
   
   Failed in:
 variant PartitionMethod:Expr in 
/home/runner/work/datafusion/datafusion/datafusion/proto/src/generated/prost.rs:2069
 variant PartitionMethod:Expr in 
/home/runner/work/datafusion/datafusion/datafusion/proto/src/generated/prost.rs:2069
   
Summary semver requires new major version: 1 major and 0 minor checks 
failed
   Finished [ 110.063s] datafusion-proto
   ```
   
   
   


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


Dandandan commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3249772156


##
datafusion/physical-expr/src/partitioning.rs:
##
@@ -133,13 +135,94 @@ impl Display for Partitioning {
 .join(", ");
 write!(f, "Hash([{phy_exprs_str}], {size})")
 }
+Partitioning::Expr(expr) => write!(f, "{expr}"),
 Partitioning::UnknownPartitioning(size) => {
 write!(f, "UnknownPartitioning({size})")
 }
 }
 }
 }
 
+/// Physical expression partitioning.
+///
+/// Partition `i` contains rows where `partition_exprs[i]` evaluates to true.
+/// The source declaring partitioning is responsible for ensuring that, for 
every
+/// row emitted, exactly one partition expression evaluates to true and that 
row
+/// is emitted by the corresponding partition. The expressions do not need to
+/// cover values that the plan cannot emit.
+///
+/// For example, a scan that can only emit rows for `2022` can declare two date
+/// partitions as:
+///
+/// ```text
+/// partition_exprs[0] = date >= 2022-01-01 AND date < 2022-07-01
+/// partition_exprs[1] = date >= 2022-07-01 AND date < 2023-01-01
+/// ```
+///
+/// This is valid even though values outside `2022` are not covered, as long as
+/// the source does not emit rows outside those ranges. It would not be valid
+/// for this plan to emit a row from `partition[i]` whose date is not within
+/// `partition_exprs[i]`, or to emit a row whose date matches multiple
+/// partition expressions.
+///
+/// More complex partitioning can be represented using normal expression
+/// composition. For example, one partition in a date and city range can be
+/// represented as `date >= 2021-01-01 AND date < 2022-07-01 AND city < 
'Boston'`.
+///
+/// NOTE: Optimizer and execution behavior for this partitioning is 
intentionally
+/// not implemented and will be introduced incrementally.
+#[derive(Debug, Clone)]
+pub struct ExprPartitioning {

Review Comment:
   Oh I see the issue already refers to it as range partititioning. Any reason 
of why not using the terminology 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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


Dandandan commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3249760484


##
datafusion/physical-expr/src/partitioning.rs:
##
@@ -133,13 +135,94 @@ impl Display for Partitioning {
 .join(", ");
 write!(f, "Hash([{phy_exprs_str}], {size})")
 }
+Partitioning::Expr(expr) => write!(f, "{expr}"),
 Partitioning::UnknownPartitioning(size) => {
 write!(f, "UnknownPartitioning({size})")
 }
 }
 }
 }
 
+/// Physical expression partitioning.
+///
+/// Partition `i` contains rows where `partition_exprs[i]` evaluates to true.
+/// The source declaring partitioning is responsible for ensuring that, for 
every
+/// row emitted, exactly one partition expression evaluates to true and that 
row
+/// is emitted by the corresponding partition. The expressions do not need to
+/// cover values that the plan cannot emit.
+///
+/// For example, a scan that can only emit rows for `2022` can declare two date
+/// partitions as:
+///
+/// ```text
+/// partition_exprs[0] = date >= 2022-01-01 AND date < 2022-07-01
+/// partition_exprs[1] = date >= 2022-07-01 AND date < 2023-01-01
+/// ```
+///
+/// This is valid even though values outside `2022` are not covered, as long as
+/// the source does not emit rows outside those ranges. It would not be valid
+/// for this plan to emit a row from `partition[i]` whose date is not within
+/// `partition_exprs[i]`, or to emit a row whose date matches multiple
+/// partition expressions.
+///
+/// More complex partitioning can be represented using normal expression
+/// composition. For example, one partition in a date and city range can be
+/// represented as `date >= 2021-01-01 AND date < 2022-07-01 AND city < 
'Boston'`.
+///
+/// NOTE: Optimizer and execution behavior for this partitioning is 
intentionally
+/// not implemented and will be introduced incrementally.
+#[derive(Debug, Clone)]
+pub struct ExprPartitioning {

Review Comment:
   https://dev.mysql.com/doc/refman/8.4/en/partitioning-range.html
   https://www.dremio.com/wiki/range-partitioning/
   
   I.e. this is a commonly used term.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add expression partitioning enum variant [datafusion]

2026-05-15 Thread via GitHub 


Dandandan commented on code in PR #22207:
URL: https://github.com/apache/datafusion/pull/22207#discussion_r3249753701


##
datafusion/physical-expr/src/partitioning.rs:
##
@@ -133,13 +135,94 @@ impl Display for Partitioning {
 .join(", ");
 write!(f, "Hash([{phy_exprs_str}], {size})")
 }
+Partitioning::Expr(expr) => write!(f, "{expr}"),
 Partitioning::UnknownPartitioning(size) => {
 write!(f, "UnknownPartitioning({size})")
 }
 }
 }
 }
 
+/// Physical expression partitioning.
+///
+/// Partition `i` contains rows where `partition_exprs[i]` evaluates to true.
+/// The source declaring partitioning is responsible for ensuring that, for 
every
+/// row emitted, exactly one partition expression evaluates to true and that 
row
+/// is emitted by the corresponding partition. The expressions do not need to
+/// cover values that the plan cannot emit.
+///
+/// For example, a scan that can only emit rows for `2022` can declare two date
+/// partitions as:
+///
+/// ```text
+/// partition_exprs[0] = date >= 2022-01-01 AND date < 2022-07-01
+/// partition_exprs[1] = date >= 2022-07-01 AND date < 2023-01-01
+/// ```
+///
+/// This is valid even though values outside `2022` are not covered, as long as
+/// the source does not emit rows outside those ranges. It would not be valid
+/// for this plan to emit a row from `partition[i]` whose date is not within
+/// `partition_exprs[i]`, or to emit a row whose date matches multiple
+/// partition expressions.
+///
+/// More complex partitioning can be represented using normal expression
+/// composition. For example, one partition in a date and city range can be
+/// represented as `date >= 2021-01-01 AND date < 2022-07-01 AND city < 
'Boston'`.
+///
+/// NOTE: Optimizer and execution behavior for this partitioning is 
intentionally
+/// not implemented and will be introduced incrementally.
+#[derive(Debug, Clone)]
+pub struct ExprPartitioning {

Review Comment:
   Isn't this the same as Range Partitioning 
https://www.waitingforcode.com/apache-spark-sql/range-partitioning-apache-spark-sql/read#range_partitioning
   
   Wouldn't it be better to use that naming?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]