-
Notifications
You must be signed in to change notification settings - Fork 803
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 encoding/decoding REE Dicts when using streaming IPC #6399
Conversation
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.
Thank you @brancz -- this makes sense to me
I have one suggestion for the test, but otherwise 🚀
arrow-ipc/src/reader/stream.rs
Outdated
|
||
let mut decoder = StreamDecoder::new(); | ||
let buf = &mut Buffer::from(buffer.as_slice()); | ||
while let Some(_) = decoder |
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 we should update the test to verify the Batch that comes back in matches the batch that was written -- aka that the data round trips
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.
makes sense, done!
@@ -375,6 +375,7 @@ impl Field { | |||
| DataType::FixedSizeList(field, _) | |||
| DataType::Map(field, _) => field.fields(), | |||
DataType::Dictionary(_, value_field) => Field::_fields(value_field.as_ref()), | |||
DataType::RunEndEncoded(_, field) => field.fields(), |
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.
👍
ea3b6b0
to
0962732
Compare
0962732
to
df2a11f
Compare
Sorry for forgetting to |
I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more |
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.
Thank you @brancz
🚀 |
Which issue does this PR close?
Closes #6398
What changes are included in this PR?
Include dictionaries within REE dicts when recursively flattening all fields of a schema.
Are there any user-facing changes?
No, just a bug fix.
@alamb @tustvold