feat: implement PhysicalOptimizerRule in FFI crate#20451
feat: implement PhysicalOptimizerRule in FFI crate#20451timsaucer merged 3 commits intoapache:mainfrom
Conversation
github-actions
bot
added
documentation
timsaucer
force-pushed
the
feat/ffi-plan-optimizer
branch
from
00ef9d6 to
5072a54
Compare
github-actions
bot
removed
documentation
timsaucer
force-pushed
the
feat/ffi-plan-optimizer
branch
from
5072a54 to
84331d9
Compare
github-actions
bot
removed
logical-expr
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thanks @timsaucer
I think nulling out the private_data field is important. Otherwise it is ready to go
| unsafe extern "C" fn release_fn_wrapper(provider: &mut FFI_PhysicalOptimizerRule) { | ||
| let private_data = | ||
| unsafe { Box::from_raw(provider.private_data as *mut RulePrivateData) }; | ||
| drop(private_data); |
There was a problem hiding this comment.
I think we should also set private_data to null here too, similarly to
| /// | ||
| /// [`SessionState::add_physical_optimizer_rule`]: https://docs.rs/datafusion/latest/datafusion/execution/session_state/struct.SessionState.html#method.add_physical_optimizer_rule | ||
| pub trait PhysicalOptimizerRule: Debug { | ||
| pub trait PhysicalOptimizerRule: Debug + std::any::Any { |
There was a problem hiding this comment.
We should probably call this PR out as a breaking API change as now all PhysicalOptimizerRules need to implement Any.
Also, I am not sure it matters, but this isn't consistent with how the other traits in this crate use Any. The others add an as_any() function. for example:
datafusion/datafusion/execution/src/cache/cache_manager.rs
Lines 210 to 212 in 0808f3a
Adding a consistent as_any method I think is still a breaking API change and I think constrains any implementation the same way, but I do think it would be better to stay consistent with the rest of DataFusion
There was a problem hiding this comment.
Though with some research it seems like this method might be nicer than the as_any methods we use elsewhere as it would be less likely that downstream crates need to be changed 🤔 (to add the as_any)
There was a problem hiding this comment.
With regard to that, may I interest you in this PR which removes almost 1200 lines? #20812
|
|
||
| use crate::physical_optimizer::FFI_PhysicalOptimizerRule; | ||
|
|
||
| /// A rule that wraps the input plan in a GlobalLimitExec with skip=0, fetch=10. |
timsaucer
force-pushed
the
feat/ffi-plan-optimizer
branch
from
6edf4c5 to
a369db2
Compare
|
🚀 |
2 participants
Which issue does this PR close?
PhysicalOptimizerRulevia FFI #20450Rationale for this change
This PR is a pure addition to implement a FFI safe PhysicalOptimizerRule. With this change downstream projects, such as
datafusion-pythoncan share optimizer rules across libraries.What changes are included in this PR?
Implement physical optimizer rule following same pattern as the rest of the FFI crate.
Are these changes tested?
Both unit and integration tests are provided.
Are there any user-facing changes?
None