-
Notifications
You must be signed in to change notification settings - Fork 1k
[thrift-remodel] Rework thrift reader API #8341
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
[thrift-remodel] Rework thrift reader API #8341
Conversation
/// Read a Thrift encoded [list] from the input protocol object. | ||
/// | ||
/// [list]: https://github.com/apache/thrift/blob/master/doc/specs/thrift-compact-protocol.md#list-and-set | ||
pub(crate) fn read_thrift_vec<'a, T, R>(prot: &mut R) -> Result<Vec<T>> |
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.
This is a big reason why this change was needed...trying to get the reading of vectors of serializable objects working with the changes to the ThriftCompactInputProtocol
changes was just too hard. Adding this function a) worked and b) made implementing the thrift macros easier.
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.
Looks good to me -- I don't fully appreciate why using a special trait rather than TryFrom
makes things easier, but on the other hand this code is simpler and more explicit so I give it a major 👍
} | ||
} | ||
|
||
/// A high performance Thrift reader that reads from a slice of bytes. |
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.
🚀
Thanks @alamb!
TBH neither do I 🤣. It was mostly down to the Anyway, onward! |
Which issue does this PR close?
Note: this targets a feature branch, not main
Rationale for this change
As I started on decoding thrift page headers, I found that the way I had been going was no longer going to work. This PR begins the process of abstracting the thrift reader to allow for other implementations.
What changes are included in this PR?
In addition to reworking the reader itself, this PR moves away from the previous
TryFrom
approach and instead adds aReadThrift
trait.Are these changes tested?
Should be covered by existing tests
Are there any user-facing changes?
Yes