Move from_unixtime, now, current_date, current_time functions to datafusion-functions #9537
Conversation
# Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs
…lain text to reflect the change. The physical plan is unchanged.
# Conflicts: # datafusion/proto/src/logical_plan/from_proto.rs
# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs
# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs
# Conflicts: # datafusion/physical-expr/src/equivalence/projection.rs
# Conflicts: # datafusion/core/tests/optimizer_integration.rs # datafusion/physical-expr/src/datetime_expressions.rs # datafusion/proto/src/logical_plan/from_proto.rs
# Conflicts: # datafusion/expr/src/built_in_function.rs # datafusion/expr/src/expr_fn.rs # datafusion/functions/src/datetime/date_bin.rs # datafusion/functions/src/datetime/mod.rs # datafusion/physical-expr/src/datetime_expressions.rs # datafusion/physical-expr/src/functions.rs # datafusion/proto/src/logical_plan/from_proto.rs
github-actions
bot
added
logical-expr
| @@ -342,7 +343,12 @@ fn test_sql(sql: &str) -> Result<LogicalPlan> { | |||
| let statement = &ast[0]; | |||
|
|
|||
| // create a logical query plan | |||
| let context_provider = MyContextProvider::default(); | |||
| let now_udf = datetime::functions() | |||
There was a problem hiding this comment.
How much of the optimizer integration test do you think needs Udfs? I am wondering maybe we could just port the tests case for now() into core/tests/sql_integration` or something and eave the rest of the optimizer_integration test in the optimizer crate?
The reason it might be good to leave the optimzer tests in the optimizer crate are
- Easier to run
cargo test -p datafusion_optimizerand run all the relevant tests - Ensure that we could still use
datafusion_optimizerwithout the function definitions
There was a problem hiding this comment.
I'll take a look. 6/20 tests use functions in the test (concat, sum, concat_ws, now(), avg). I can move just those over for sure. I wish this test dependency issue wasn't such an problem in Rust - most of the problems I've seen migrating the functions have been test related.
There was a problem hiding this comment.
Thinking about this more - the issue is not test related because we can use dev dependencies. The issue is publishing to crates.io because cargo publishes with dev-dependencies and has been for many many years according to the bug report. Would it be possible to just fix that by the use of --no-dev-deps or something similar during publishing?
🤔
There was a problem hiding this comment.
I think moving the tests to the core (what this PR does) is probably the best solution for now, as it correctly reflects the dependencies (the optimizer tests are testing behavior of functions that are not available to optimizer)
There was a problem hiding this comment.
I moved all but the tests that required now() back to the datafusion/optimizer/tests folder.
| _args: Vec<Expr>, | ||
| info: &dyn SimplifyInfo, | ||
| ) -> Result<ExprSimplifyResult> { | ||
| let now_ts = info.execution_props().query_execution_start_time; |
# Conflicts: # datafusion/expr/src/built_in_function.rs # datafusion/expr/src/expr_fn.rs # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs # datafusion/proto/src/logical_plan/from_proto.rs # datafusion/proto/src/logical_plan/to_proto.rs
…s to the core/tests folder, leave the rest in place.
| @@ -342,7 +343,12 @@ fn test_sql(sql: &str) -> Result<LogicalPlan> { | |||
| let statement = &ast[0]; | |||
|
|
|||
| // create a logical query plan | |||
| let context_provider = MyContextProvider::default(); | |||
| let now_udf = datetime::functions() | |||
There was a problem hiding this comment.
I think moving the tests to the core (what this PR does) is probably the best solution for now, as it correctly reflects the dependencies (the optimizer tests are testing behavior of functions that are not available to optimizer)
|
🚀 - thank you @Omega359 |
Which issue does this PR close?
Closes #9466
Rationale for this change
Functions are migrated to the new crate and use the new patterns as described in #9286 & #9216
What changes are included in this PR?
Code, tests moved to new location, references updated.
Are these changes tested?
Yes
Are there any user-facing changes?
No