feat: array-empty
#7313
feat: array-empty
#7313
Conversation
github-actions
bot
added
logical-expr
| @@ -2363,6 +2363,16 @@ from flatten_table; | |||
| [1, 2, 3] [1, 2, 3, 4, 5, 6] [1, 2, 3] [1.0, 2.1, 2.2, 3.2, 3.3, 3.4] | |||
| [1, 2, 3, 4, 5, 6] [8] [1, 2, 3] [1.0, 2.0, 3.0, 4.0, 5.0, 6.0] | |||
|
|
|||
| query I | |||
There was a problem hiding this comment.
You should add some comments (like # empty scalar function #1).
| @@ -548,6 +551,7 @@ impl BuiltinScalarFunction { | |||
| "The {self} function can only accept list as the first argument" | |||
| ), | |||
| }, | |||
| BuiltinScalarFunction::ArrayEmpty => Ok(UInt8), | |||
There was a problem hiding this comment.
The function can return only 2 values (0 and 1), so it is reasonable to use Boolean instead of UInt8
| let array = as_list_array(&args[0])?; | ||
| let mut builder = UInt8Array::builder(1); | ||
|
|
||
| for arr in array.iter() { |
There was a problem hiding this comment.
I don't like using builder method for array functions. I think you can use map method and collect the results as BooleanArray:
It seems to me like:
| for arr in array.iter() { | |
| array.iter().map(|arr| arr.is_empty()).collect::<BooleanArray>() |
P.S. arr is Option, so you should consider None case too.
|
Thank you, @Weijun-H! |
| } | ||
| }) | ||
| .collect::<BooleanArray>(); | ||
| // let mut builder = UInt8Array::builder(1); |
There was a problem hiding this comment.
remember to cleanup this
There was a problem hiding this comment.
Looks good to me other than one stray println!
Thank you @Weijun-H @izveigor @jayzhan211 and @crepererum . This is a great team effort
| } | ||
| }) | ||
| .collect::<BooleanArray>(); | ||
| println!("\n"); |
There was a problem hiding this comment.
This println! seems like it is an oversight.
| println!("\n"); |
| @@ -986,6 +986,26 @@ macro_rules! general_repeat_list { | |||
| }}; | |||
| } | |||
|
|
|||
| /// Array_empty SQL function | |||
| pub fn array_empty(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
| if args[0].as_any().downcast_ref::<NullArray>().is_some() { | |||
There was a problem hiding this comment.
I think this doesn't catch all nulls. This only works within the test setup because array_empty(NULL) creates a NULL array. I think this will fail w/:
SELECT array_empty(a)
FROM VALUES(NULL, make_array(), make_array(1)) sub (a);I suggest you remove this check and change the else part within array.iter().map(...) to None (from Some(true)).
There was a problem hiding this comment.
I agree, but that doesn't mean we have to add more immature code, esp. when the fix is rather simple (I would argue the fixed code is even simpler than the current version).
There was a problem hiding this comment.
I suggest you remove this check and change the else part within
array.iter().map(...)toNone(fromSome(true)).
It cannot handle all NULL array cases in this way, but I think we can wait #7142 to make this pr better.
|
Thanks everyone! |
Which issue does this PR close?
Closes #7290
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?