fix: Capture nullability in Values node planning
#14472
Conversation
github-actions
bot
added
sql
|
Thank you @rkrishn7 -- I marked this PR as a draft as it seems a CI test is failing. Thank you! |
|
FYI @gatesn |
|
Thanks @alamb. Will fix it up when I'm back from work! |
|
|
| } | ||
|
|
||
| fn infer_inner( | ||
| mut values: Vec<Vec<Expr>>, | ||
| field_types: &[DataType], | ||
| field_nullabilities: &[bool], |
There was a problem hiding this comment.
Another way that you could do this is to pass in the fields
like
fn infer_inner(
mut values: Vec<Vec<Expr>>,
field_types: &[&Field],
schema: &DFSchema
) -> Result<Self> {And then instead of making a new field
.map(|(j, (data_type, nullable))| {
// naming is following convention https://www.postgresql.org/docs/current/queries-values.html
let name = &format!("column{}", j + 1);
Field::new(name, data_type.clone(), *nullable)
})Do something like
.map(|(j, field)| {
// naming is following convention https://www.postgresql.org/docs/current/queries-values.html
field.clone().with_name(format!("column{}", j + 1))
})There was a problem hiding this comment.
Yes, good call out!
We end up cloning the data type to construct the field, so I just created a small container which encapsulates the naming logic as well. Think it cleans things up a bit!
| @@ -255,7 +255,7 @@ insert into table_without_values(id, id) values(3, 3); | |||
| statement error Arrow error: Cast error: Cannot cast string 'zoo' to value of Int64 type | |||
| insert into table_without_values(name, id) values(4, 'zoo'); | |||
|
|
|||
| statement error Error during planning: Column count doesn't match insert query! | |||
| statement error Error during planning: Inconsistent data length across values list: got 2 values in row 0 but expected 1 | |||
There was a problem hiding this comment.
These seem significantly better errors ❤️
|
I merged up from main to get a clean CI run |
Which issue does this PR close?
What changes are included in this PR?
Includes field nullability from table schema when planning a
Valuesnode duringINSERT INTO ... VALUESAre these changes tested?
Existing tests have been updated, but it would be great to figure out additional tests to add.
Are there any user-facing changes?
N/A