-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix array_append with null array #8333
Conversation
cc @jayzhan211 PTAL, I find this one when porting tests |
afb9a3c
to
fd94178
Compare
Signed-off-by: veeupup <code@tanweime.com>
fd94178
to
18de0bf
Compare
Signed-off-by: veeupup <code@tanweime.com>
5221b69
to
b0ccf19
Compare
if data_types.contains(&DataType::Null) { | ||
Ok(()) | ||
} else { | ||
let types = args.iter().map(|arg| arg.data_type()).collect::<Vec<_>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems types
variable is not used at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad, its it err message.
we may want to avoid code duplication for
let types = args.iter().map(|arg| arg.data_type()).collect::<Vec<_>>();
plan_err!("{name} received incompatible types: '{types:?}'.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match x {
1 => "ok",
2 if true => "ok",
_ => "err"
};
I think we should not add more and more type coercion or checking in |
Agree with you! I'll close this PR to avoid to make |
Which issue does this PR close?
fix todo in
array.slt
, now we can doarray_append
correctly with null.and fix
fn check_datatypes
, this function contains a bug that when input arrays sequences are different:(DataType::Null, DataType::Int64)
used to return unexcepted array while(DataType::Int64, DataType::Null)
returnsOk(())
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?