Uh oh!
There was an error while loading. Please reload this page.
Introduce array_except function - #8135
Conversation
| parse_expr(&args[0], registry)?, | ||
| parse_expr(&args[1], registry)?, | ||
| )), | ||
| ScalarFunction::ArrayIntersect => Ok(array_intersect( |
There was a problem hiding this comment.
Do we need to enhance the check during CI? maybe this is a little bug?
| ## array_except | ||
| statement ok |
There was a problem hiding this comment.
Unlike other tests, I try a new style of test, keep creating table and drop table close together, hope this is easier for review.
There was a problem hiding this comment.
this is choice of style, imho no-table tests are easier to read.
query ?
select array_expect([], []), array_except(....)
----
[], ...
There was a problem hiding this comment.
Well, we still need table to test multi-rows cases. The new style I mean is not create table in the beginning of this large file and drop table at the end of the file, but when we are done we drop it.
There was a problem hiding this comment.
the multirow still possible to do without creating a table
select ... from (select 1 a, 'asdf' b union all select 2 b, 'zxcv' b)
thats the matter of style of course.
There was a problem hiding this comment.
We cant differentiate empty array and null currently. Both of them are null type So we can fix them in another PR
There was a problem hiding this comment.
I also prefer the new style (create/query/drop table statements are organized more tightly). This will make tests more clear.
3846b70 to
6a291c2CompareThere was a problem hiding this comment.
Thanks @jayzhan211 please also add tests with empty arrays
And since we have introduced the new built in function we need to get it documented inscalar_functions.md and expressions.md
49108ae to
edf8618CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
cd0b846 to
b990b47CompareVeeupup
commented
Nov 14, 2023
LGTM! |
comphead
left a comment
There was a problem hiding this comment.
lgtm thanks @jayzhan211
Please create a followup ticket to differentiate empty array and null, that sounds important
alamb
commented
Nov 14, 2023
There appears to be a non trivial number of conflicts in this PR now |
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
b990b47 to
218a299CompareSigned-off-by: jayzhan211 <jayzhan211@gmail.com>
alamb
commented
Nov 17, 2023
I took the liberty of merging up from main to resolve a merge conflict |
Which issue does this PR close?
Closes#6979
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?