-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add serialization of ScalarValue::Struct
#3536
Conversation
9a28f02
to
1c7b3fe
Compare
1c7b3fe
to
cc2b408
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, although I would question the need for is_null. Whilst it is faithful to ScalarValue, perhaps ScalarValue shouldn't contain the additional Option? 🤔
@@ -739,6 +739,13 @@ message IntervalMonthDayNanoValue { | |||
int64 nanos = 3; | |||
} | |||
|
|||
message StructValue { | |||
// encode null explicitly to distinguish a struct with no fields (is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no fields there can't be any values, null or otherwise? I therefore think we could drop this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try and remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assert that there are always at least one Field
/ value and removed explicit support is_null in 88370bf
Benchmark runs are scheduled for baseline = 41b59cf and contender = b4c0601. b4c0601 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Draft as it builds on #3535Which issue does this PR close?
Closes #3531
Rationale for this change
See #3531
What changes are included in this PR?
Changes:
ScalarValue::Struct
Are there any user-facing changes?
No