Move nullif and isnan to datafusion-functions
#9216
Conversation
github-actions
bot
added
logical-expr
| Gcd, | ||
| /// lcm, Least common multiple | ||
| Lcm, | ||
| /// isnan |
There was a problem hiding this comment.
One benefit is that the implementation of these functions are consolidated, rather than having them spread all over
| use arrow::datatypes::DataType; | ||
| //! "core" DataFusion functions | ||
| mod nullif; |
There was a problem hiding this comment.
To add new functions, we can add the appropriate module and entry in this file
| signature: Signature, | ||
| } | ||
|
|
||
| /// Currently supported types by the nullif function. |
There was a problem hiding this comment.
this code is just moved from various other places in the codebase. There is no new logic
| /// $RETURN_TYPE: the type of array to return | ||
| /// $FUNC: the function to apply to each element of $ARG | ||
| /// | ||
| macro_rules! make_function_scalar_inputs_return_type { |
There was a problem hiding this comment.
This is copied (with more documentation) from datafusion-physical_expr. Once we move all the functions we can remove the original copy
| Ok(DataType::Boolean) | ||
| } | ||
|
|
||
| fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> { |
There was a problem hiding this comment.
Likewise all this code is moved from elsewhere, it is not new
| expr::{self, InList, Sort, WindowFunction}, | ||
| factorial, find_in_set, flatten, floor, from_unixtime, gcd, gen_range, initcap, | ||
| instr, isnan, iszero, lcm, left, levenshtein, ln, log, log10, log2, | ||
| instr, iszero, lcm, left, levenshtein, ln, log, log10, log2, |
There was a problem hiding this comment.
the expr_fn's are still available (via datafusion_functions exports)
| statement error | ||
| SELECT nullif(1); | ||
| ---- | ||
| DataFusion error: Failed to coerce arguments for NULLIF |
There was a problem hiding this comment.
I improved the error message slightly as well
move nullif
alamb
force-pushed
the
alamb/extract_is_nan
branch
from
a355d4a to
8e5f3ad
Compare
| planner.statement_to_plan(ast.pop_front().unwrap()) | ||
| } | ||
|
|
||
| fn make_udf(name: &'static str, args: Vec<DataType>, return_type: DataType) -> ScalarUDF { |
There was a problem hiding this comment.
The sql integration test needs function definitions, to resolve function names, but doesn't actually invoke them
| /// $ARG: ArrayRef | ||
| /// $NAME: name of the argument (for error messages) | ||
| /// $ARRAY_TYPE: the type of array to cast the argument to | ||
| macro_rules! downcast_arg { |
There was a problem hiding this comment.
I remember to move those downcast macros :)
|
Thank you @comphead 🙏 |
2 participants
Which issue does this PR close?
Part of #8045
See also #9100
Rationale for this change
The high level rationale is described in #8045
In this PR I am trying to set up the pattern for how functions can be moved to the
datafusion-functionscrate so we can continue the wholesale movement.I specifically wanted
coreto unblock the movement ofarrow_cast-- #9143 (comment) from @brayanjulsWhat changes are included in this PR?
isnanfunction todatafusion-functionscrate, math_expressionsdatafusion-functionscrate, core_expressionsAre these changes tested?
Yes, covered by existing tests and new CI test
Are there any user-facing changes?
I don't think so