-
Notifications
You must be signed in to change notification settings - Fork 1.5k
ScalarUDF with zero arguments should be provided with one null array as parameter #9031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,8 @@ pub struct ScalarFunctionExpr { | |
// and it specifies the effect of an increase or decrease in | ||
// the corresponding `arg` to the function value. | ||
monotonicity: Option<FuncMonotonicity>, | ||
// Whether this function can be invoked with zero arguments | ||
supports_zero_argument: bool, | ||
} | ||
|
||
impl Debug for ScalarFunctionExpr { | ||
|
@@ -79,13 +81,15 @@ impl ScalarFunctionExpr { | |
args: Vec<Arc<dyn PhysicalExpr>>, | ||
return_type: DataType, | ||
monotonicity: Option<FuncMonotonicity>, | ||
supports_zero_argument: bool, | ||
) -> Self { | ||
Self { | ||
fun, | ||
name: name.to_owned(), | ||
args, | ||
return_type, | ||
monotonicity, | ||
supports_zero_argument, | ||
} | ||
} | ||
|
||
|
@@ -138,9 +142,12 @@ impl PhysicalExpr for ScalarFunctionExpr { | |
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> { | ||
// evaluate the arguments, if there are no arguments we'll instead pass in a null array | ||
// indicating the batch size (as a convention) | ||
let inputs = match (self.args.len(), self.name.parse::<BuiltinScalarFunction>()) { | ||
let inputs = match ( | ||
self.args.is_empty(), | ||
self.name.parse::<BuiltinScalarFunction>(), | ||
) { | ||
// MakeArray support zero argument but has the different behavior from the array with one null. | ||
(0, Ok(scalar_fun)) | ||
(true, Ok(scalar_fun)) | ||
if scalar_fun | ||
.signature() | ||
.type_signature | ||
|
@@ -149,6 +156,11 @@ impl PhysicalExpr for ScalarFunctionExpr { | |
{ | ||
vec![ColumnarValue::create_null_array(batch.num_rows())] | ||
} | ||
// If the function supports zero argument, we pass in a null array indicating the batch size. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I never fully understood why this didn't just check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. Changed to |
||
// This is for user-defined functions. | ||
(true, Err(_)) if self.supports_zero_argument => { | ||
vec![ColumnarValue::create_null_array(batch.num_rows())] | ||
} | ||
_ => self | ||
.args | ||
.iter() | ||
|
@@ -175,6 +187,7 @@ impl PhysicalExpr for ScalarFunctionExpr { | |
children, | ||
self.return_type().clone(), | ||
self.monotonicity.clone(), | ||
self.supports_zero_argument, | ||
))) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -578,8 +578,9 @@ fn roundtrip_builtin_scalar_function() -> Result<()> { | |
"acos", | ||
fun_expr, | ||
vec![col("a", &schema)?], | ||
DataType::Int64, | ||
DataType::Float64, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The existing test is not correct at all. Previously the roundtrip test passes because But in this PR, |
||
None, | ||
false, | ||
); | ||
|
||
let project = | ||
|
@@ -617,6 +618,7 @@ fn roundtrip_scalar_udf() -> Result<()> { | |
vec![col("a", &schema)?], | ||
DataType::Int64, | ||
None, | ||
false, | ||
); | ||
|
||
let project = | ||
|
Uh oh!
There was an error while loading. Please reload this page.