Use try_new when casting between structs to propagate error
#5226
Conversation
| @@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool { | |||
| (Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true, | |||
| // Utf8 to decimal | |||
| (Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true, | |||
| (Struct(from_fields), Struct(to_fields)) => { | |||
There was a problem hiding this comment.
Btw, "tab" was used in the merged code. It made incorrect ident in diff display. I fixed it to use space.
There was a problem hiding this comment.
Do we still need to update can_cast_types?
There was a problem hiding this comment.
You mean tab -> space fix or the added comment?
There was a problem hiding this comment.
I think it is better to fix the tab with space, otherwise the diff always shows a wrong indent.
The comment is good to have to explain why nullability is ignored there.
There was a problem hiding this comment.
Sorry, I saw modifications and thought you hadn't updated this 😅
|
Perhaps we could use StructArray::try_new which will validate nullability and return an error, if we aren't already. I think this would still preserve the invariant that nullable columns don't obtain nulls? Or to phrase it differently, the array constructors already verify this, so perhaps this is sufficient?? |
Ah, that's good to know Thanks. |
viirya
changed the title
try_new when casting between structs
viirya
changed the title
try_new when casting between structstry_new when casting between structs to propagate error
viirya
changed the title
try_new when casting between structs to propagate errortry_new when casting between structs to propagate error
|
Thank you @tustvold |
Which issue does this PR close?
Closes #.
Rationale for this change
Different to casting from primitive types, casting between structs needs to consider nullability as the fields of structs have nullability properties. We use
StructArray::newwhen casting structs for now which simply fails. We should useStructArray::try_newto propagate the error.What changes are included in this PR?
Are there any user-facing changes?