-
Notifications
You must be signed in to change notification settings - Fork 839
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
fix: coerce_primitive for serde decoded data #5101
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -61,7 +61,15 @@ impl<O: OffsetSizeTrait> ArrayDecoder for StringArrayDecoder<O> { | |||||||||||||||||||
TapeElement::Number(idx) if coerce_primitive => { | ||||||||||||||||||||
data_capacity += tape.get_string(idx).len(); | ||||||||||||||||||||
} | ||||||||||||||||||||
_ => return Err(tape.error(*p, "string")), | ||||||||||||||||||||
TapeElement::I64(n) | TapeElement::I32(n) if coerce_primitive => { | ||||||||||||||||||||
data_capacity += n.to_string().len(); | ||||||||||||||||||||
} | ||||||||||||||||||||
TapeElement::F32(n) | TapeElement::F64(n) if coerce_primitive => { | ||||||||||||||||||||
data_capacity += n.to_string().len(); | ||||||||||||||||||||
} | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This avoids serializing to a string multiple times, this could be improved if it becomes a bottleneck, but this is probably good enough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This improvement looks good. |
||||||||||||||||||||
_ => { | ||||||||||||||||||||
return Err(tape.error(*p, "string")); | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -89,6 +97,12 @@ impl<O: OffsetSizeTrait> ArrayDecoder for StringArrayDecoder<O> { | |||||||||||||||||||
TapeElement::Number(idx) if coerce_primitive => { | ||||||||||||||||||||
builder.append_value(tape.get_string(idx)); | ||||||||||||||||||||
} | ||||||||||||||||||||
TapeElement::I64(n) | TapeElement::I32(n) if coerce_primitive => { | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The handling of I64 is incorrect here, this will only use the low bits. There are some examples elsewhere in how to interpret this tape element There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about the design here. Why we don't use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the TapeElement would then need to increase in size from the current 64-bits to 128-bits (for alignment reasons), tape decoding is exceptionally hot and so concerns like this actually matter 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I get it. Just for performance. 😺 |
||||||||||||||||||||
builder.append_value(n.to_string()); | ||||||||||||||||||||
} | ||||||||||||||||||||
TapeElement::F32(n) | TapeElement::F64(n) if coerce_primitive => { | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you do the same for f64 please 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
done. |
||||||||||||||||||||
builder.append_value(n.to_string()); | ||||||||||||||||||||
} | ||||||||||||||||||||
_ => unreachable!(), | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
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.
Perhaps we could test with some numbers that are larger than i32::MAX, i.e. require i64
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.
done, add i32::MAX and like i64::MAX tests.