Move create_physical_expr to phy-expr-common #1
#10144
Conversation
jayzhan211
changed the title
create_physical_expr to phy-expr-commoncreate_physical_expr to phy-expr-common #1
There was a problem hiding this comment.
Thanks @jayzhan211 -- this looks like nice progress to me. I had a few questions / comments but in general very nice. It will be amazing to see the aggregate / window functions pulled into their own crates and the APIs clarified
| pub mod column; | ||
| pub mod datum; |
There was a problem hiding this comment.
I agree it makes sense to have the datum / scalars / cast stuff in physical expr common
| @@ -0,0 +1,362 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Likewise I think it makes sense to have the interval arithmetic in physical-expr-common too
There was a problem hiding this comment.
If we do that so, can Expr's still access interval arithmetic?
There was a problem hiding this comment.
If I didn't miss any pub use to make it backward compatible, there is no change from others
| fn reverse_tuple<T, U>((first, second): (T, U)) -> (U, T) { | ||
| (second, first) | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
is there a reason this entire file wasn't migrated over?
There was a problem hiding this comment.
I'm not sure if I need all of them, so I only move what I need for now
There was a problem hiding this comment.
ExprIntervalGraphNode is not used for creating expr, but in execution plan
| @@ -106,6 +106,14 @@ pub fn create_physical_expr( | |||
| input_dfschema: &DFSchema, | |||
| execution_props: &ExecutionProps, | |||
| ) -> Result<Arc<dyn PhysicalExpr>> { | |||
| use datafusion_physical_expr_common::physical_expr::create_physical_expr as create_physical_expr_common; | |||
|
|
|||
| // Temporary solution, after all the logic is moved to common, we can remove this function | |||
There was a problem hiding this comment.
Can you add a ticket reference in this comment too so anyone else who stumbles on this can review the backstory / reference?
| if res.is_ok() { | ||
| return res; | ||
| } | ||
|
|
There was a problem hiding this comment.
For the cases below (like Expr::Alias) that were moved, can we please remove them from this list? I think it would make it clearer that that code is now dead (and make it easier to track what remains to port
There was a problem hiding this comment.
It is not possible yet since create_physical_expr_common failed most of the time because many other expression is not yet supported, so we fall back to the original one.
There was a problem hiding this comment.
easier to track what remains to port
I think we can check what is still commented out in common
| @@ -23,8 +23,8 @@ use std::{any::Any, sync::Arc}; | |||
| use crate::expressions::datum::{apply, apply_cmp}; | |||
There was a problem hiding this comment.
My hope / dream for the binary operators are that over time we would be able to extract them into functions (e.g so we would have a + b --> add(a, b)
However, I can't think if any good justification for this yet and I think we have bigger things to focus on than the semantics of operators
| /// * `input_dfschema` - The DataFusion schema for the input, used to resolve `Column` references | ||
| /// to qualified or unqualified fields by name. | ||
| #[allow(clippy::only_used_in_recursion)] | ||
| pub fn create_physical_expr( |
There was a problem hiding this comment.
If you move all the the code that is commented out in this function to physical-expr-common what will be left in physical-expr?
There was a problem hiding this comment.
If you move all the the code that is commented out in this function to
physical-expr-commonwhat will be left inphysical-expr?
code for physical-plan are not. For example, ExprIntervalGraph in cp_solver is not moved, it is used in physical-plan
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
jayzhan211
force-pushed
the
move-phy-expr-to-common
branch
from
d79775e
to
6908732
Compare
alamb
dismissed
their stale review
I think I am missing something -- see #10188 (review) for details
Which issue does this PR close?
Part 1 of #10074
I plan to move it in several PRs
Most of the code is for binary.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?