You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jayshrivastava (via GitHub)" <gi...@apache.org> on 2026/08/03 20:58:46 UTC
[PR] add ExecutionPlan::dynamic_filters() method [datafusion]
jayshrivastava opened a new pull request, #24068:
URL: https://github.com/apache/datafusion/pull/24068
## Which issue does this PR close?
- Closes: https://github.com/apache/datafusion/issues/23814
- Closes: https://github.com/datafusion-contrib/datafusion-distributed/issues/584
## Rationale for this change
External users who wish to propagate dynamic filter updates across network boundaries need to know in which direction updates need to flow. Thus `ExecutionPlan::apply_expressions` is not enough. The proposal in https://github.com/apache/datafusion/issues/23814 is to provide a separate API for dynamic filter producers.
## What changes are included in this PR?
This change adds a new method
## Are these changes tested?
Yes.
## Are there any user-facing changes?
Here's
--
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] add ExecutionPlan::dynamic_expressions() method [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3719266958
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -165,6 +166,20 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
check_default_invariants(self, check)
}
+ /// Returns the dynamic expressions produced by this plan node.
+ ///
+ /// A dynamic expression is produced when this node updates or completes its
+ /// runtime state during execution. Expressions that this node only consumes
+ /// must not be returned. This method is shallow and does not include dynamic
+ /// expressions produced by child plans.
+ ///
+ /// Each returned expression must have a [`PhysicalExpr::expression_id`]
+ /// since all dynamic expressions such as [`DynamicFilterPhysicalExpr`]
+ /// have an expression id.
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ Vec::new()
+ }
Review Comment:
Do you think a better name would be `dynamic_expressions_produced`, or something that indicates it only returns dynamic filters that are produced? I think this name can be a bit misleading and might be interpreted as returning any dynamic filters the node is holding, whether it is a consumer holding the dynamic filters it will consume or a producer holding the dynamic filters it will produce. I had to read [this](https://github.com/apache/datafusion/issues/23814#issuecomment-5051433022) comment to understand the motivation behind it
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1330,6 +1330,16 @@ impl ExecutionPlan for HashJoinExec {
vec![&self.left, &self.right]
}
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
If this function is supposed to be the promotion of the non trait function `dynamic_filter_expr`, should we remove `dynamic_filter_expr`?
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3730424687
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -165,6 +166,20 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
check_default_invariants(self, check)
}
+ /// Returns the dynamic expressions produced by this plan node.
+ ///
+ /// A dynamic expression is produced when this node updates or completes its
+ /// runtime state during execution. Expressions that this node only consumes
+ /// must not be returned. This method is shallow and does not include dynamic
+ /// expressions produced by child plans.
+ ///
+ /// Each returned expression must have a [`PhysicalExpr::expression_id`]
+ /// since all dynamic expressions such as [`DynamicFilterPhysicalExpr`]
+ /// have an expression id.
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ Vec::new()
+ }
Review Comment:
Agreed. Just pushed
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1330,6 +1330,16 @@ impl ExecutionPlan for HashJoinExec {
vec![&self.left, &self.right]
}
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
Done. It's marked as deprecated
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3732058680
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
> Could this ever return something other than DynamicFilterPhysicalExpr?
To be clear, the answer is no. It's always `DynamicFilterPhysicalExpr` today
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
> Could this ever return something other than DynamicFilterPhysicalExpr?
To be clear, the answer is no. It's always `DynamicFilterPhysicalExpr` today
--
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] add ExecutionPlan::dynamic_filters() method [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5171715758
<!-- 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.1.0 (current)
Built [ 107.055s] (current)
Parsing datafusion v54.1.0 (current)
Parsed [ 0.038s] (current)
Building datafusion v54.1.0 (baseline)
Built [ 106.741s] (baseline)
Parsing datafusion v54.1.0 (baseline)
Parsed [ 0.037s] (baseline)
Checking datafusion v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.933s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 216.683s] datafusion
Building datafusion-ffi v54.1.0 (current)
Built [ 58.926s] (current)
Parsing datafusion-ffi v54.1.0 (current)
Parsed [ 0.062s] (current)
Building datafusion-ffi v54.1.0 (baseline)
Built [ 58.703s] (baseline)
Parsing datafusion-ffi v54.1.0 (baseline)
Parsed [ 0.065s] (baseline)
Checking datafusion-ffi v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.338s] 223 checks: 221 pass, 1 fail, 1 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field FFI_ExecutionPlan.dynamic_expressions in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:55
field ForeignLibraryModule.create_exec_with_dynamic_expressions in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:112
--- warning repr_c_plain_struct_fields_reordered: struct fields reordered in repr(C) struct ---
Description:
A public repr(C) struct had its fields reordered. This can change the struct's memory layout, possibly breaking FFI use cases that depend on field position and order.
ref: https://doc.rust-lang.org/reference/type-layout.html#reprc-structs
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/repr_c_plain_struct_fields_reordered.ron
Failed in:
FFI_ExecutionPlan.with_new_children moved from position 3 to 4, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:57
FFI_ExecutionPlan.name moved from position 4 to 5, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:61
FFI_ExecutionPlan.execute moved from position 5 to 6, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:65
FFI_ExecutionPlan.repartitioned moved from position 6 to 7, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:71
FFI_ExecutionPlan.metrics moved from position 7 to 8, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:80
FFI_ExecutionPlan.partition_statistics moved from position 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:86
FFI_ExecutionPlan.clone moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:93
FFI_ExecutionPlan.release moved from position 10 to 11, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:96
FFI_ExecutionPlan.private_data moved from position 11 to 12, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:100
FFI_ExecutionPlan.library_marker_id moved from position 12 to 13, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:105
ForeignLibraryModule.create_exec_with_statistics moved from position 15 to 16, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:114
ForeignLibraryModule.create_table_with_statistics moved from position 16 to 17, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:116
ForeignLibraryModule.create_physical_optimizer_rule moved from position 17 to 18, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:119
ForeignLibraryModule.create_context_aware_optimizer_rule moved from position 18 to 19, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:121
ForeignLibraryModule.version moved from position 19 to 20, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:123
Summary semver requires new major version: 1 major and 0 minor checks failed
Warning produced 1 major and 0 minor level warnings
Finished [ 119.341s] datafusion-ffi
Building datafusion-physical-plan v54.1.0 (current)
Built [ 36.179s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.144s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 36.112s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.148s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.956s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 74.720s] datafusion-physical-plan
```
</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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3732056068
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
I kept it as `Arc<dyn PhysicalExpr>` and named it `dynamic_expressions_produced` instead of `dynamic_filters_produced` to avoid leaking the abstraction. `DynamicFilterPhysicalExpr` is just a `PhysicalExpr`. One way this facade helps is that we can return `FFI_PhysicalExpr` in the FFI layer (there's no `FFI_DynamicFilterPhysicalExpr`). It also helps avoid a breaking API change if someone were to add new types of dynamic expressions.
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3734967003
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
makes sense
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5219633652
Exciting to see this get across the line!
--
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] add ExecutionPlan::dynamic_expressions() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3729744418
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -474,15 +498,18 @@ impl ExecutionPlan for ForeignExecutionPlan {
#[cfg(any(test, feature = "integration-tests"))]
pub mod tests {
- use datafusion_physical_plan::Partitioning;
+ #[cfg(test)]
Review Comment:
why does this need another cfg test (isn't it already in a cfg test?)
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1330,6 +1330,16 @@ impl ExecutionPlan for HashJoinExec {
vec![&self.left, &self.right]
}
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
yes please.
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3734972670
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -355,6 +355,13 @@ if tracking.contains_dynamic_filter() {
}
```
+### Concrete execution-plan dynamic-filter getters are deprecated
Review Comment:
I don't think this is necessary to put in the upgrade guide (the functions are deprecated and the compiler will explain that with the corresponding error message)
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3736022023
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -355,6 +355,13 @@ if tracking.contains_dynamic_filter() {
}
```
+### Concrete execution-plan dynamic-filter getters are deprecated
Review Comment:
Done. The conflict was in this file so that's resolved as well.
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3731887672
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1087,6 +1087,10 @@ impl AggregateExec {
}
/// Returns the dynamic filter expression for this aggregate, if set.
+ #[deprecated(
+ since = "55.0.0",
+ note = "Use ExecutionPlan::dynamic_expressions_produced instead"
Review Comment:
👍
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
Could this ever return something other than `DynamicFilterPhysicalExpr`? Specifically I am wonering if this should be
```rust
fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn DynamicFilterPhysicalExpr>> {
```
instead of
```rust
fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
```
🤔
--
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] add ExecutionPlan::dynamic_expressions() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5206821071
> I guess we could use apply_expressions, downcast to the node, and then decide which nodes are producers and which are consumers. However, that might not be the cleanest approach, since the list of producers might vary.
Yes. Downcasting becomes a bit more tricky if custom `ExecutionPlan` nodes want to produce dynamic filters
--
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] add ExecutionPlan::dynamic_expressions() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3729763307
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -165,6 +166,20 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
check_default_invariants(self, check)
}
+ /// Returns the dynamic expressions produced by this plan node.
+ ///
+ /// A dynamic expression is produced when this node updates or completes its
+ /// runtime state during execution. Expressions that this node only consumes
+ /// must not be returned. This method is shallow and does not include dynamic
+ /// expressions produced by child plans.
+ ///
+ /// Each returned expression must have a [`PhysicalExpr::expression_id`]
+ /// since all dynamic expressions such as [`DynamicFilterPhysicalExpr`]
+ /// have an expression id.
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ Vec::new()
+ }
Review Comment:
I agree making it clear this is about what dynamic expressions the plan produces in the name is 💯
--
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] add ExecutionPlan::dynamic_expressions() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3729759795
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1330,6 +1330,16 @@ impl ExecutionPlan for HashJoinExec {
vec![&self.left, &self.right]
}
+ fn dynamic_expressions(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
> should we remove dynamic_filter_expr?
Specifically I think we should deprecate it following the deprecation guidelines
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3732062667
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
But the invariant is that we the return values have a non null `expression_id`, indicating they are dynamic. Adding an invariant that they all downcast to `DynamicFilterPhysicalExpr` feels too strong.
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5219159506
Thanks for reviewing @alamb!
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #24068:
URL: https://github.com/apache/datafusion/pull/24068
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5219762459
We are making progress, bit by bit 🥇
--
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] add ExecutionPlan::dynamic_expressions() method [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5206178736
Looks like there are some CI failures.
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3730503727
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -474,15 +498,18 @@ impl ExecutionPlan for ForeignExecutionPlan {
#[cfg(any(test, feature = "integration-tests"))]
pub mod tests {
- use datafusion_physical_plan::Partitioning;
+ #[cfg(test)]
Review Comment:
It's needed right now because `DynamicFilterPhysicalExpr` was unused when compiling under `integration-tests`. I'll lifted the usage into a helper method so we don't need the `#[cfg(test)]`
--
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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#discussion_r3732056068
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1945,6 +1964,16 @@ impl ExecutionPlan for AggregateExec {
vec![&self.input]
}
+ fn dynamic_expressions_produced(&self) -> Vec<Arc<dyn PhysicalExpr>> {
Review Comment:
I kept it as `Arc<dyn PhysicalExpr>` and named it `dynamic_expressions_produced` instead of `dynamic_filters_produced` to avoid leaking the abstraction. `DynamicFilterPhysicalExpr` is just a `PhysicalExpr`. One way this facade helps is that we can return `FFI_PhysicalExpr` in the FFI layer (there's no `FFI_DynamicFilterPhysicalExpr`). It also helps avoid a breaking API change if someone were to add new types of dynamic expressions which need to be returned 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: 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] add ExecutionPlan::dynamic_expressions_produced() method [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24068:
URL: https://github.com/apache/datafusion/pull/24068#issuecomment-5211176632
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24068?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `94.50549%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.06%. Comparing base ([`541caab`](https://app.codecov.io/gh/apache/datafusion/commit/541caab85a265c104888f1988fe54b08fa022f53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`66de95d`](https://app.codecov.io/gh/apache/datafusion/commit/66de95d2ae698eb0b2a9629db3af541aa1400598?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 72 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24068?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/aggregates/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL21vZC5ycw==) | 78.94% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/ffi/src/execution\_plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&filepath=datafusion%2Fffi%2Fsrc%2Fexecution_plan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mZmkvc3JjL2V4ZWN1dGlvbl9wbGFuLnJz) | 96.36% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tafusion/physical-plan/src/joins/hash\_join/exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fjoins%2Fhash_join%2Fexec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9qb2lucy9oYXNoX2pvaW4vZXhlYy5ycw==) | 86.66% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/execution\_plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fexecution_plan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9leGVjdXRpb25fcGxhbi5ycw==) | 98.03% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/sorts/sort.rs](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fsorts%2Fsort.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9zb3J0cy9zb3J0LnJz) | 96.87% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24068 +/- ##
==========================================
+ Coverage 80.84% 81.06% +0.21%
==========================================
Files 1096 1106 +10
Lines 373936 380704 +6768
Branches 373936 380704 +6768
==========================================
+ Hits 302313 308617 +6304
- Misses 53584 53864 +280
- Partials 18039 18223 +184
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24068?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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