Avoid use of flatbuffers::size_prefixed_root, fix validation error in arrow-flight
#7109
Conversation
bkietz
force-pushed
the
7058-avoid-use-of-size_prefixed_root
branch
2 times, most recently
from
d689e17 to
c225a02
Compare
bkietz
force-pushed
the
7058-avoid-use-of-size_prefixed_root
branch
from
c225a02 to
f9e091b
Compare
|
The change looks good to me, I've tested in in https://github.com/XiangpengHao/liquid-cache, and it indeed fixes the problem it tries to fix. cc @alamb |
| @@ -2617,4 +2619,43 @@ mod tests { | |||
| let err = read_ipc_with_decoder(buf).unwrap_err(); | |||
| assert_eq!(err.to_string(), expected_err); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_roundtrip_schema() { | |||
There was a problem hiding this comment.
Strangely this test passes for me on main (without the changes in this PR)
So I don't think it covers the issue somehow. I am looking into it a bit more
There was a problem hiding this comment.
Thank you @bkietz and @XiangpengHao
I took the liberty of merging this PR up from main as well as adding the test case from @XiangpengHao 's original issue #7058
The reason the test in this PR might not be failing might be related to how it sets up the dictionary tracker where arrow-flight does this:
let mut dict_tracker =
writer::DictionaryTracker::new_with_preserve_dict_id(false, options.preserve_dict_id());arrow-rs/arrow-flight/src/lib.rs
Lines 147 to 148 in fc6936a
| buffer.split_at(4) | ||
| }; | ||
|
|
||
| let len = <i32>::from_le_bytes(len.try_into().unwrap()); |
There was a problem hiding this comment.
this are very reasonable checks 👍
alamb
changed the title
alamb
changed the title
flatbuffers::size_prefixed_root, fix validation error in arrow-flight
Which issue does this PR close?
Rationale for this change
flatbuffers::size_prefixed_root does not seem to do what was originally expected. Replacing it with more explicit code for handling an encapsulated message prefix seems to resolve the case of interest.
What changes are included in this PR?
Usage of
size_prefixed_root( there was only one call, introduced in #2586 ) is replaced with manual prefixed length wrangling. A unit test is addedAre there any user-facing changes?
No
Maybe the error messages?