fix string_to_array handle non string delimiter#32081
fix string_to_array handle non string delimiter#32081ptravers merged 7 commits intoMaterializeInc:mainfrom
Conversation
ParkMyCar
left a comment
ParkMyCar
left a comment
There was a problem hiding this comment.
Approving to unblock! Would love to see if changing the function signature would work here though
bkirwi
added
release-blocker
| | Least | ||
| | MakeTimestamp | ||
| | ArrayIndex { .. } | ||
| | StringToArray |
There was a problem hiding this comment.
I think having introduces_nulls return false was correct. Or can you show a case where it returns null despite none of its arguments being null? (true is also ok from a correctness point of view, just not as tight as it could be, so it's better to return false.)
(I admit that the doc comment of introduces_nulls/propagates_nulls is not precise enough. I have a todo to enhance these comments, but it's on page 32 of my 66-page todo doc...)
There was a problem hiding this comment.
yeah, so basically this is a hack as there's an issue where to return a null at all the function must:
- set propagates_null to true
- set introduces_null to true
if 1 then for any input parameter being null we auto return null. which is incorrect as parameter two being null does not result in null.
my other solution was to add a third flag to avoid the skipping the function and returning null. @ParkMyCar felt that this was overkill. so we went with this slight hack on introduces_null set to true.
| StringToArray => { | ||
| ScalarType::Array(Box::new(ScalarType::String)).nullable(input_types[0].nullable) | ||
| } | ||
| StringToArray => ScalarType::Array(Box::new(ScalarType::String)).nullable(true), |
There was a problem hiding this comment.
I might be missing something, but as far as I can see, it only returns null if the first argument is null. This can only happen if the first argument is nullable. This would mean that the original version was correct.
(Note that some elements of the result array can be null in other cases as well. However, this does not mean that the result itself is nullable.)
There was a problem hiding this comment.
I agree. responded below.
4 participants
fixes non string handling in string_to_array. currently causing panics.
Motivation
errors in buildkite: https://buildkite.com/materialize/nightly/builds/11690#annotation-0195f3b5-b1bd-440b-8c7e-a445219baf63-error
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.