You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2024/04/23 09:10:02 UTC
[PR] Avoid creating null array for empty args [datafusion]
jayzhan211 opened a new pull request, #10193:
URL: https://github.com/apache/datafusion/pull/10193
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576190000
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -403,123 +398,6 @@ async fn test_user_defined_functions_with_alias() -> Result<()> {
Ok(())
}
-#[derive(Debug)]
-pub struct RandomUDF {
Review Comment:
We have `RandomFunc`, they are the same so remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576108924
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
I think we don't need it now. I don't have a strong opinion on whether to keep it or not.
> namely to return a single row array (or ColumnarValue)
functions that take 0 args are not restricted to return a single row array. I think they can have a function that returns anything as long as is `Result<ColumnarValue>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576665520
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -403,123 +398,6 @@ async fn test_user_defined_functions_with_alias() -> Result<()> {
Ok(())
}
-#[derive(Debug)]
-pub struct RandomUDF {
- signature: Signature,
-}
-
-impl RandomUDF {
- pub fn new() -> Self {
- Self {
- signature: Signature::any(0, Volatility::Volatile),
- }
- }
-}
-
-impl ScalarUDFImpl for RandomUDF {
- fn as_any(&self) -> &dyn std::any::Any {
- self
- }
-
- fn name(&self) -> &str {
- "random_udf"
- }
-
- fn signature(&self) -> &Signature {
- &self.signature
- }
-
- fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
- Ok(Float64)
- }
-
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- // This udf is always invoked with zero argument so its argument
- // is a null array indicating the batch size.
- ColumnarValue::Array(array) if array.data_type().is_null() => array.len(),
- _ => {
- return Err(datafusion::error::DataFusionError::Internal(
- "Invalid argument type".to_string(),
- ))
- }
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.1..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
- }
-}
-
-/// Ensure that a user defined function with zero argument will be invoked
-/// with a null array indicating the batch size.
-#[tokio::test]
-async fn test_user_defined_functions_zero_argument() -> Result<()> {
- let ctx = SessionContext::new();
-
- let schema = Arc::new(Schema::new(vec![Field::new(
- "index",
- DataType::UInt8,
- false,
- )]));
-
- let batch = RecordBatch::try_new(
- schema,
- vec![Arc::new(UInt8Array::from_iter_values([1, 2, 3]))],
- )?;
-
- ctx.register_batch("data_table", batch)?;
-
- let random_normal_udf = ScalarUDF::from(RandomUDF::new());
- ctx.register_udf(random_normal_udf);
-
- let result = plan_and_collect(
- &ctx,
- "SELECT random_udf() AS random_udf, random() AS native_random FROM data_table",
- )
- .await?;
-
- assert_eq!(result.len(), 1);
- let batch = &result[0];
- let random_udf = batch
- .column(0)
- .as_any()
- .downcast_ref::<Float64Array>()
- .unwrap();
- let native_random = batch
- .column(1)
- .as_any()
- .downcast_ref::<Float64Array>()
- .unwrap();
-
- assert_eq!(random_udf.len(), native_random.len());
-
- let mut previous = -1.0;
- for i in 0..random_udf.len() {
- assert!(random_udf.value(i) >= 0.0 && random_udf.value(i) < 1.0);
- assert!(random_udf.value(i) != previous);
- previous = random_udf.value(i);
- }
-
- Ok(())
-}
-
-#[tokio::test]
-async fn deregister_udf() -> Result<()> {
Review Comment:
This function is just moved , the test remains
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1575925764
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
The reason why we need to add special case for `make_array` is that we need to differentiate empty array `[]` and single null array `[null]`, previously we avoid additional null array by filter out `make_array`. Now, I think it is even not a good idea to add specializaton to effect every functions, so remove it at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577722216
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
We have test like
```
query II
SELECT octet_length(uuid()), length(uuid())
----
36 36
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577034922
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
Nice catch! Let me think about how to design it, I would prefer something like `support_random` to specialize `random()` case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "liukun4515 (via GitHub)" <gi...@apache.org>.
liukun4515 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1580663479
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
file a issue for this: https://github.com/apache/datafusion/issues/10247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "liukun4515 (via GitHub)" <gi...@apache.org>.
liukun4515 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577661127
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
Do we have any method to hide the special behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#issuecomment-2074663133
How about adding num_rows to invoke function? Let user decide how to deal
with args and num_rows?
On Wed, Apr 24, 2024, 6:27 PM Kun Liu ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In datafusion/physical-expr/src/scalar_function.rs
> <https://github.com/apache/datafusion/pull/10193#discussion_r1577661127>:
>
> >
> // evaluate the function
> match self.fun {
> - ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
> + ScalarFunctionDefinition::UDF(ref fun) => {
> + if fun.support_randomness() {
> + fun.invoke_no_args(batch.num_rows())
>
> Do we have any method to hide the special behavior?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/datafusion/pull/10193#discussion_r1577661127>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ADZCLR3GAV44GZ2J76JUXXDY66CITAVCNFSM6AAAAABGUPBNIWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJZGQ4TANZSGA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576108924
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
I think we don't need it now, I don't have a strong opinion on whether to keep it or not.
> namely to return a single row array (or ColumnarValue)
functions that take 0 args are not restricted to return a single row array. I think they can have a function that returns anything as long as is `Result<ColumnarValue>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1579753336
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
I don't quite understand `support_randomness` was needed. I gave it a try locally and I think we can avoid `support_randomness` like this: https://github.com/jayzhan211/arrow-datafusion/pull/2
Let me know what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#issuecomment-2079191977
Here is a PR with tests that validate we get the right answer for multiple rows: https://github.com/apache/datafusion/issues/10247
Thanks again @jayzhan211 for cleaning this up and @liukun4515 for your reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1575980985
##########
datafusion/functions/src/math/random.rs:
##########
@@ -69,12 +69,13 @@ impl ScalarUDFImpl for RandomFunc {
}
}
-/// Random SQL function
fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return exec_err!("Expect random function to take no param");
Review Comment:
This might be more consistent with the other changes in this PR:
```suggestion
return exec_err!("Expect {} function to take no parameters", self.name());
```
##########
datafusion/functions/src/math/pi.rs:
##########
@@ -63,9 +63,10 @@ impl ScalarUDFImpl for PiFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- if !matches!(&args[0], ColumnarValue::Array(_)) {
- return exec_err!("Expect pi function to take no param");
- }
+ if !args.is_empty() {
+ return exec_err!("Expect {} function to take no param", self.name());
Review Comment:
```suggestion
return exec_err!("Expect {} function to take no parameters", self.name());
```
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -434,15 +434,10 @@ impl ScalarUDFImpl for RandomUDF {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- // This udf is always invoked with zero argument so its argument
- // is a null array indicating the batch size.
- ColumnarValue::Array(array) if array.data_type().is_null() => array.len(),
- _ => {
- return Err(datafusion::error::DataFusionError::Internal(
- "Invalid argument type".to_string(),
- ))
- }
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return internal_err!("Invalid argument type");
Review Comment:
It might be good to have the test match the rest of the functions for consistency
```suggestion
return exec_err!("Expect function to take no parameters");
```
##########
datafusion/functions/src/string/uuid.rs:
##########
@@ -61,9 +61,10 @@ impl ScalarUDFImpl for UuidFunc {
/// Prints random (v4) uuid values per row
/// uuid() = 'a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11'
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect uuid function to take no param"),
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return exec_err!("Expect {} function to take no param", self.name());
Review Comment:
```suggestion
return exec_err!("Expect {} function to take no parameters", self.name());
```
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
So is the thinking `support_zero_argument` is no longer needed? Should that field be removed entirely now?
If all the existing tests pass this certainly seems like a reasonable special case to remove (maybe we could update the documentation on `ScalarUDFImpl` to explain how to handle funcitons that take zero arguments -- namely to return a single row array (or ColumnarValue)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576121743
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
I think `support_zero_argument` can be handled in `signature check`, and actually I can't find a test that failed on `Expect {} function to take no parameters` easily. I think we have a good reason to remove `support_zero_argument` now.
`signature check`
https://github.com/apache/datafusion/blob/da82cece8181d5af0d38243229ecb2b4722cb4a9/datafusion/expr/src/signature.rs#L219-L228
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "liukun4515 (via GitHub)" <gi...@apache.org>.
liukun4515 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577595754
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
Do we test the `uuid` function? i think the `uuid` has the same attribute like `random`
cc @jayzhan211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577075655
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
I didn't see the message, it is also a good idea.
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
I didn't see the message then, it is also a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576121743
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
I think `support_zero_argument` can be handled in `signature check`, we have a good reason to remove `support_zero_argument` now.
`signature check`
https://github.com/apache/datafusion/blob/da82cece8181d5af0d38243229ecb2b4722cb4a9/datafusion/expr/src/signature.rs#L219-L228
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1580245132
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
Functions that include randomness don't imply 0 args at all. I think it is better to call `invoke_no_args` if it is no args 💯
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "liukun4515 (via GitHub)" <gi...@apache.org>.
liukun4515 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1580661398
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
> We have test like
>
> ```
> query II
> SELECT octet_length(uuid()), length(uuid())
> ----
> 36 36
> ```
I means the `uuid` function is also `invoked once per row rather than once per batch` like `random()` function mentioned by alamb.
I test the function in the spark
```
spark-sql> desc test;
col1 int NULL
Time taken: 0.065 seconds, Fetched 1 row(s)
spark-sql> select * from test;
1
2
spark-sql> select *,uuid() from test;
1 6b04b66c-2e6c-4925-8b18-a9d51d5ed80a
2 3e0be0c2-9ff2-422a-8f3f-5cdb6551264b
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576291793
##########
datafusion/functions/src/math/pi.rs:
##########
@@ -62,12 +60,10 @@ impl ScalarUDFImpl for PiFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- if !matches!(&args[0], ColumnarValue::Array(_)) {
- return exec_err!("Expect pi function to take no param");
- }
- let array = Float64Array::from_value(std::f64::consts::PI, 1);
- Ok(ColumnarValue::Array(Arc::new(array)))
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
I think signature check is enough, so just ignore args
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576114361
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
Maybe we can remove `support_zero_argument `, add it back if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576665672
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -615,6 +493,22 @@ async fn test_user_defined_functions_cast_to_i64() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn deregister_udf() -> Result<()> {
Review Comment:
👍
##########
datafusion/expr/src/udf.rs:
##########
@@ -322,11 +322,6 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
/// The function will be invoked passed with the slice of [`ColumnarValue`]
/// (either scalar or array).
///
- /// # Zero Argument Functions
- /// If the function has zero parameters (e.g. `now()`) it will be passed a
- /// single element slice which is a a null array to indicate the batch's row
- /// count (so the function can know the resulting array size).
- ///
Review Comment:
Can we please leave a note about what is required to implement `Zero Argument Functions`? I think the expectation is that the output is a single `ColumnarValue::Scalar`, rather than an Array
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
I think the idea here is that expectation is that `rand` is invoked *once per row* rather than *once per batch*. And the only way it knew how many rows to make is to get a null array in 🤔
For example, when I run `datafusion-cli` from this PR to call random() the same value is returned for each row:
```sql
> create table foo as values (1), (2), (3), (4), (5);
0 row(s) fetched.
Elapsed 0.018 seconds.
> select column1, random() from foo;
+---------+--------------------+
| column1 | random() |
+---------+--------------------+
| 1 | 0.9594375709000513 |
| 2 | 0.9594375709000513 |
| 3 | 0.9594375709000513 |
| 4 | 0.9594375709000513 |
| 5 | 0.9594375709000513 |
+---------+--------------------+
5 row(s) fetched.
Elapsed 0.012 seconds.
```
But I expect that each row has a different value for `random()`
However, since none of the tests failed, clearly we have a gap in test coverage 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577039606
##########
datafusion/functions/src/math/random.rs:
##########
@@ -64,45 +61,9 @@ impl ScalarUDFImpl for RandomFunc {
Ok(Float64)
}
- fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- random(args)
- }
-}
-
-/// Random SQL function
-fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
- };
- let mut rng = thread_rng();
- let values = iter::repeat_with(|| rng.gen_range(0.0..1.0)).take(len);
- let array = Float64Array::from_iter_values(values);
- Ok(ColumnarValue::Array(Arc::new(array)))
-}
-
-#[cfg(test)]
-mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
- use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
-
- use crate::math::random::random;
-
- #[test]
- fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
- .expect("failed to initialize function random")
- .into_array(1)
- .expect("Failed to convert to array");
- let floats =
- as_float64_array(&array).expect("failed to initialize function random");
-
- assert_eq!(floats.len(), 1);
- assert!(0.0 <= floats.value(0) && floats.value(0) < 1.0);
+ fn invoke(&self, _args: &[ColumnarValue]) -> Result<ColumnarValue> {
Review Comment:
What about adding a `invoke_no_args(num_rows: usize)` method to the `ScalarUDFImpl` -- with a default implementation that returns "not implemented" error
That might make it clear what was happening and would provide clear semantics about what to do in this case 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "liukun4515 (via GitHub)" <gi...@apache.org>.
liukun4515 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1577658690
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
@jayzhan211 @alamb
in order to resolve no arg function issue, we introduce two interface `support_randomness` and `invoke_no_args` for `ScalarUDFImpl` , I have no good idea to resolve the no arg function issue.
But it really increases barrier to implement a new scalarUDF function, we need to know the attribute of the new function.
If the attribute of the function is random, we must implement the interface of `nvoke_no_args`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#issuecomment-2079306390
Thanks @alamb and @liukun4515
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #10193:
URL: https://github.com/apache/datafusion/pull/10193
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1580245132
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
I think it is nice! Functions that include randomness don't imply 0 args at all. I think it is better to call `invoke_no_args` if it is no args 💯
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1580746996
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
I forgot that `make_array` accept any number of args including 0. :(
```rust
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
make_scalar_function(make_array_inner)(args)
}
fn invoke_no_args(&self, _number_rows: usize) -> Result<ColumnarValue> {
make_scalar_function(make_array_inner)(&[])
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1580746996
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,25 +137,21 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
- true if self.supports_zero_argument && self.name != "make_array" => {
- vec![ColumnarValue::create_null_array(batch.num_rows())]
- }
- _ => self
- .args
- .iter()
- .map(|e| e.evaluate(batch))
- .collect::<Result<Vec<_>>>()?,
- };
+ let inputs = self
+ .args
+ .iter()
+ .map(|e| e.evaluate(batch))
+ .collect::<Result<Vec<_>>>()?;
// evaluate the function
match self.fun {
- ScalarFunctionDefinition::UDF(ref fun) => fun.invoke(&inputs),
+ ScalarFunctionDefinition::UDF(ref fun) => {
+ if fun.support_randomness() {
+ fun.invoke_no_args(batch.num_rows())
Review Comment:
I forgot that `make_array` accept any number of args including 0, so we need to somehow specialize the function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1575925764
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
The reason why we need to add special case for `make_array` is that we need to differentiate empty array `[]` and single null array `[null]`, previously we avoid additional null array by filter out `make_array`. Now, I think it is even not a good idea to add specialization to effect every functions, so remove it at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576121743
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ 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.is_empty() {
- // If the function supports zero argument, we pass in a null array indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior from the array with one null.
Review Comment:
I think `support_zero_argument` can be handled in `signature check`, and actually I can't find a test that failed on `Expect {} function to take no parameters` easily. I think we have a good reason to remove `support_zero_argument` now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576097589
##########
datafusion/functions/src/math/random.rs:
##########
@@ -69,12 +69,13 @@ impl ScalarUDFImpl for RandomFunc {
}
}
-/// Random SQL function
fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return exec_err!("Expect random function to take no param");
Review Comment:
I did this before, and I noticed that `random` is used in test so I need to move it out as an independent function, but adding `name` as another argument looks so weird, so I keep it like this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576139980
##########
datafusion/functions/src/math/random.rs:
##########
@@ -69,12 +69,13 @@ impl ScalarUDFImpl for RandomFunc {
}
}
-/// Random SQL function
fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return exec_err!("Expect random function to take no param");
Review Comment:
we can even return ScalarValue::F64 instead of Array. Let me fix these too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] ScalarUDF: Remove `supports_zero_argument` and avoid creating null array for empty args [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1576155672
##########
datafusion/functions/src/math/random.rs:
##########
@@ -83,19 +84,13 @@ fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
#[cfg(test)]
mod test {
- use std::sync::Arc;
-
- use arrow::array::NullArray;
-
use datafusion_common::cast::as_float64_array;
- use datafusion_expr::ColumnarValue;
- use crate::math::random::random;
+ use super::*;
#[test]
fn test_random_expression() {
- let args = vec![ColumnarValue::Array(Arc::new(NullArray::new(1)))];
- let array = random(&args)
+ let array = random(&[])
Review Comment:
remove this test, since it is already covered
https://github.com/apache/datafusion/blob/da82cece8181d5af0d38243229ecb2b4722cb4a9/datafusion/sqllogictest/test_files/expr.slt#L788-L790
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org