-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix encoding/decoding REE Dicts when using streaming IPC #6399
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
Conversation
alamb
left a comment
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
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!
arrow-schema/src/field.rs
Outdated
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 |
alamb
left a comment
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