Move AggregateExpr, PhysicalExpr and PhysicalSortExpr to physical-expr-core
#9926
Conversation
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
| /// [`create_physical_expr`]: https://docs.rs/datafusion/latest/datafusion/physical_expr/fn.create_physical_expr.html | ||
| /// | ||
| /// # Example: Create `PhysicalExpr` from `Expr` | ||
| /// ```ignore |
There was a problem hiding this comment.
Since datafusion_physical_expr::create_physical_expr should stay in physical-expr, so we can't run the doc test here.
There was a problem hiding this comment.
I suggest we move the examples to create_physical_expr so it continues to run in tests and link to it via html. Something like
See [create_physical_expr] for examples of creating `PhysicalExpr` from `Expr`:
[create_physical_expr]: https://docs.rs/datafusion/latest/datafusion/physical_expr/fn.create_physical_expr.html
alamb
changed the title
AggregateExpr, PhysicalExpr and PhysicalSortExpr to physical-expr-core
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @jayzhan211 -- this looks quite good to me. It is a very exciting step forward
I suggest renaming this crate to datafusion-physical-expr-common to follow the same pattern of datafusion-common but I don't think that is required
cc @mustafasrepo and @berkaysynnada
| This crate is a submodule of DataFusion that provides the core functionality of physical expressions. | ||
| Like `PhysicalExpr` or `PhysicalSortExpr` and related things. |
There was a problem hiding this comment.
Here is a very minor suggestion:
| This crate is a submodule of DataFusion that provides the core functionality of physical expressions. | |
| Like `PhysicalExpr` or `PhysicalSortExpr` and related things. | |
| This crate is a submodule of DataFusion that provides shared APIs for implementing | |
| physical expressions such as `PhysicalExpr` and `PhysicalSortExpr`. |
| /// [`create_physical_expr`]: https://docs.rs/datafusion/latest/datafusion/physical_expr/fn.create_physical_expr.html | ||
| /// | ||
| /// # Example: Create `PhysicalExpr` from `Expr` | ||
| /// ```ignore |
There was a problem hiding this comment.
I suggest we move the examples to create_physical_expr so it continues to run in tests and link to it via html. Something like
See [create_physical_expr] for examples of creating `PhysicalExpr` from `Expr`:
[create_physical_expr]: https://docs.rs/datafusion/latest/datafusion/physical_expr/fn.create_physical_expr.html
| use arrow::datatypes::Field; | ||
| use datafusion_common::{not_impl_err, Result}; | ||
| use datafusion_expr::{Accumulator, GroupsAccumulator}; | ||
| pub use datafusion_physical_expr_core::aggregate::AggregateExpr; |
There was a problem hiding this comment.
💯 for backwards compatibility
| pub use cast::{cast, cast_with_options, CastExpr}; | ||
| pub use column::{col, Column, UnKnownColumn}; | ||
| pub use column::UnKnownColumn; | ||
| pub use datafusion_physical_expr_core::expressions::column::{col, Column}; |
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
mustafasrepo
left a comment
mustafasrepo
left a comment
There was a problem hiding this comment.
LGTM! Thanks @jayzhan211 for this PR.
|
Thanks @jayzhan211 and @mustafasrepo 🚀 |
3 participants
Which issue does this PR close?
Closes #9921.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?