Add plan_any/plan_all to ExprPlanner to decouple functions from sql crate#22967
Add plan_any/plan_all to ExprPlanner to decouple functions from sql crate#22967Jefffrey wants to merge 1 commit into
plan_any/plan_all to ExprPlanner to decouple functions from sql crate#22967Conversation
| ))) | ||
| } | ||
|
|
||
| fn plan_any(&self, args: RawAnyAllExpr) -> Result<PlannerResult<RawAnyAllExpr>> { |
There was a problem hiding this comment.
this implementation code is copied essentially verbatim from sql/src/expr/mod.rs; the only modifications being some renaming of variables (for any it called them left/right which are now aligned to needle/haystack) and some plumbing to make it work with the signatures of plan_any/plan_all
| chrono = { workspace = true } | ||
| datafusion-common = { workspace = true, features = ["sql"] } | ||
| datafusion-expr = { workspace = true, features = ["sql"] } | ||
| datafusion-functions-nested = { workspace = true, features = ["sql"] } |
| array_position(haystack.clone(), lit(ScalarValue::Null), lit(1i64)) | ||
| .is_not_null(); | ||
|
|
||
| let decisive_condition = match op { |
There was a problem hiding this comment.
also moving this planning code into datafusion-functions-nested should allow us to more easily create dedicated UDFs for any/all without needing to expose these UDFs beyond the crate
- e.g. instead of needing to create new UDFs
array_has_all_eq/array_has_all_gtthat must be public for the planning code to reach it (fromsql/src/expr/mod.rs) we can keep it private within this crate
related:
| let right_expr = self.sql_to_expr(*right, schema, planner_context)?; | ||
| plan_any_op(left_expr, right_expr, &compare_op) | ||
| let needle = self.sql_to_expr(*left, schema, planner_context)?; | ||
| let haystack = self.sql_to_expr(*right, schema, planner_context)?; |
There was a problem hiding this comment.
one part i'm not sure about was whether to include this subquery case as part of this plan_any/plan_all call; maybe its better to just name these as plan_any_nested/plan_all_nested to differentiate it from the subquery planning, which we'll keep in datafusion-sql?
Which issue does this PR close?
Rationale for this change
sql crate toml states that it should not depend on the functions crates:
datafusion/datafusion/sql/Cargo.toml
Lines 49 to 59 in 127731b
However on line 59 it is still depending on the nested functions crate, due to any/all planning support:
datafusion/datafusion/sql/src/expr/mod.rs
Lines 1263 to 1272 in 127731b
array_hasfunction. Aim to decouple these crates via usingExprPlannertrait.What changes are included in this PR?
Introduce
plan_allandplan_anytoExprPlanner, and move the implementation code intodatafusion-functions-nested, removing dependency ofdatafusion-functions-nestedfromdatafusion-sqlAre these changes tested?
Existing tests.
Are there any user-facing changes?
Hopefully not? Theres new methods on
ExprPlannerbut they have default implementations, and if usingNestedFunctionPlanner(which should be there by default I believe) it'll still plan any/all correctly.