feat: roundtrip FixedSizeList Scalar to protobuf #8239
Conversation
github-actions
bot
added
documentation
wjones127
removed
documentation
| match value { | ||
| Value::ListValue(_) => Self::List(arr.to_owned()), | ||
| Value::FixedSizeListValue(_) => Self::FixedSizeList(arr.to_owned()), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
I think in theory this code could be reached with malformed input -- it might be nice to return an error rather than panic
There was a problem hiding this comment.
Which panic are you referring to?
If you mean the unreachable!(), this statement is inside a match guard that guarantees value is one of these two, so I think it is truly unreachable.
There was a problem hiding this comment.
FYI. For the sake of code robustness, I agree with @alamb 's suggestion to return an error instead of unreachable. If someone accidentally modifies the match branch, this panic might get exposed.
There was a problem hiding this comment.
I see -- I missed the fact that this is in a match outer branch. I agree this code is really unreachable. Perhaps we can make it more robust per @yukkit 's and my suggestion in some later PR
|
Thanks @wjones127 and @yukkit |
Which issue does this PR close?
Closes #8224.
Rationale for this change
This scalar type was missing from the protobuf serialization. Adding it for completeness.
What changes are included in this PR?
Updates protobuf to add a FixedSizeList scalar variant. I've re-used the same serialization mechanism as list, to minimize the changes needed.
Are these changes tested?
I've added a roundtrip test, which I think should be sufficient. I'm not deeply familiar with the proto module codebase, so if you have additional suggestions for tests, I am all ears.
Are there any user-facing changes?
This should be a backwards compatible change to the protobuf messages.