-
Notifications
You must be signed in to change notification settings - Fork 721
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 StructArrayReader handling nested lists (#1651) #1700
Conversation
.map(|buf| unsafe { buf.typed_data() }) | ||
// Children definition levels should describe the same parent structure, | ||
// so return key_reader only | ||
self.key_reader.get_def_levels() |
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.
Drive by fix, part of #1699
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 am a big fan of less unsafe
👍
c5a8d4b
to
230eb38
Compare
230eb38
to
f679322
Compare
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 @tustvold
I found this easier to review by ignoring whitespace https://github.com/apache/arrow-rs/pull/1700/files?w=1
As I am not a super expert in this code, I can't say I fully grok it but if it fixes the file from @kesavkolla sounds good to me.
I had one question on the tests, but I suspect it is my own mis-understanding
|
||
// Safety: the buffer is always treated as `u16` in the code below | ||
let def_level_data = unsafe { def_level_data_buffer.typed_data_mut() }; |
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.
+1 for removing some unsafe
parquet/src/arrow/array_reader.rs
Outdated
None, | ||
])); | ||
|
||
let nulls = Buffer::from([0b00000111]); |
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.
doesn't this set the first three elements in the struct array to NULL
?
That doesn't seem consistent with the structure created in the comments above
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.
Will rename to validity, it is an arrow null mask... I agree the naming is perpetually confusing
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.
aaah!
.map(|buf| unsafe { buf.typed_data() }) | ||
// Children definition levels should describe the same parent structure, | ||
// so return key_reader only | ||
self.key_reader.get_def_levels() |
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 am a big fan of less unsafe
👍
Which issue does this PR close?
Closes #1651.
Rationale for this change
See ticket
What changes are included in this PR?
Fixes handling of nested lists within StructArrayReader
Are there any user-facing changes?
Files that used to fail to parse, now parse correctly