FIx VALUES tuples type casts #12104
FIx VALUES tuples type casts #12104
Conversation
Previously, the plan created for a VALUES list would attempt to auto detect the type of each column. Unfortunately, for UInt64 columns this results in cases where a large UInt64 that exceeds the range of Int64 causing cast errors when it is attempted to be cast down to a UInt64.
github-actions
bot
added
sql
| if column_types.is_empty() { | ||
| LogicalPlanBuilder::values(values)?.build() | ||
| } else { | ||
| LogicalPlanBuilder::values_with_types(values, column_types)?.build() |
There was a problem hiding this comment.
I am not convinced this is a good idea, as it will lead to inconsistent behavior or statements that should be equivalent. See #12103 (comment)
There was a problem hiding this comment.
I agree with @findepi -- #12103 (comment)
Thank you @davisp for this PR -- let us know what you think about the alternate proposal
There was a problem hiding this comment.
I'm definitely on board in making this more general. It didn't feel super awesome to be fixing things this specifically in the first place, it was just the first thing that came to mind.
@alamb You mentioned in your comment on #12103 that there are other coercion patterns to follow, can you point me at anything vaguely similar to reference? Skimming the DataType docs, I'm not immediately seeing how I'd handle the expansion correctly.
There was a problem hiding this comment.
In general, the coercsion code is in https://github.com/apache/datafusion/tree/main/datafusion/expr/src/type_coercion
This has a reasonable description:
https://docs.rs/datafusion/latest/datafusion/logical_expr/type_coercion/index.html
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
Which issue does this PR close?
Closes #12103
Rationale for this change
The general idea here is to provide the target schema fields to the logical plan builder so that it can avoid the need to guess on VALUES tuple column types.
What changes are included in this PR?
This updates the SQL planner to accept a list of column data types for VALUE tuple columns which are then used to inform the logical plan builder which types each column should be cast to.
Are these changes tested?
I've added a test in the sqllogictests crate showing the behavior is fixed. I suspect there will be a request for more tests, but I am uncertain on where they should be placed. I did attempt to try and add a test to the sql crate's tests, but AFAICT those are all stateless which means more specific tests should probably live in the expr crate. However, I wasn't able to find an obviously place to add tests for these changes.
Are there any user-facing changes?
Some queries that would have previously failed, will no longer fail.