Fix values with different data types caused failure #10445
Conversation
github-actions
bot
added
logical-expr
There was a problem hiding this comment.
Look good to me -- thank you @b41sh and @jayzhan211
I had some small code suggestions but I don't think they are required to merge this PR
| let data_type = row[j].get_type(&empty_schema)?; | ||
| if data_type != *field_type { | ||
| row[j] = Expr::Cast(Cast { | ||
| expr: Box::new(row[j].clone()), | ||
| data_type: field_type.clone(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you can write this more concisely like
| let data_type = row[j].get_type(&empty_schema)?; | |
| if data_type != *field_type { | |
| row[j] = Expr::Cast(Cast { | |
| expr: Box::new(row[j].clone()), | |
| data_type: field_type.clone(), | |
| }); | |
| } | |
| } | |
| row[j] = std::mem::take(&mut row[j]).cast_to(&field_type, &empty_schema)?; |
The std::mem::take is needed to avoid requiring a clone
| } | ||
| if let Some(prev_type) = common_type { | ||
| // get common type of each column values. | ||
| match values_coercion(&data_type, &prev_type) { |
There was a problem hiding this comment.
Minor: I think you could write the same thing like this slightly more concisely (and I don't think we need new_type.clone())
if let Some(prev_type) = common_type {
// get common type of each column values.
let Some(new_type) = values_coercion(&data_type, &prev_type) else {
return plan_err!("Inconsistent data type across values list at row {i} column {j}. Was {prev_type} but found {data_type}");
};
common_type = Some(new_type);
} else {
common_type = Some(data_type.clone());
}
| @@ -2140,4 +2156,25 @@ mod tests { | |||
| ); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_values() -> Result<()> { | |||
There was a problem hiding this comment.
I think we don't need this since creating table in slt already covered it
Very good suggestion, I have revised it, thank you for your review @alamb |
|
Thanks again @b41sh ! |
* Fix values with different data types caused failure * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * add `list_coercion` * fix review suggestions
Which issue does this PR close?
Closes #10440
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?