You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/13 13:40:55 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #1700: Fix StructArrayReader handling nested lists (#1651)
tustvold opened a new pull request, #1700:
URL: https://github.com/apache/arrow-rs/pull/1700
_Draft as builds on #1682_
# 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #1700: Fix StructArrayReader handling nested lists (#1651)
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1700:
URL: https://github.com/apache/arrow-rs/pull/1700#discussion_r873651342
##########
parquet/src/arrow/array_reader.rs:
##########
@@ -1564,12 +1541,74 @@ mod tests {
.collect::<Vec<bool>>()
);
assert_eq!(
- Some(vec![0, 1, 1, 1, 1].as_slice()),
+ Some(vec![0, 1, 2, 3, 1].as_slice()),
struct_array_reader.get_def_levels()
);
assert_eq!(
Some(vec![0, 1, 1, 1, 1].as_slice()),
struct_array_reader.get_rep_levels()
);
}
+
+ #[test]
+ fn test_struct_array_reader_list() {
+ use arrow::datatypes::Int32Type;
+ // [
+ // {foo: [1, 2, null],
+ // {foo: []},
+ // {foo: null},
+ // null,
+ // ]
+
+ let expected_l =
+ Arc::new(ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
+ Some(vec![Some(1), Some(2), None]),
+ Some(vec![]),
+ None,
+ None,
+ ]));
+
+ let nulls = Buffer::from([0b00000111]);
Review Comment:
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
##########
parquet/src/arrow/array_reader.rs:
##########
@@ -742,80 +737,59 @@ impl ArrayReader for StructArrayReader {
.collect::<Vec<ArrayData>>(),
);
- if self.struct_def_level != 0 {
+ if self.nullable {
// calculate struct def level data
- let buffer_size = children_array_len * size_of::<i16>();
- let mut def_level_data_buffer = MutableBuffer::new(buffer_size);
- def_level_data_buffer.resize(buffer_size, 0);
- // Safety: the buffer is always treated as `u16` in the code below
- let def_level_data = unsafe { def_level_data_buffer.typed_data_mut() };
Review Comment:
+1 for removing some `unsafe`
##########
parquet/src/arrow/array_reader/map_array.rs:
##########
@@ -154,15 +150,15 @@ impl ArrayReader for MapArrayReader {
}
fn get_def_levels(&self) -> Option<&[i16]> {
- self.def_level_buffer
- .as_ref()
- .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()
Review Comment:
I am a big fan of less `unsafe` 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #1700: Fix StructArrayReader handling nested lists (#1651)
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #1700:
URL: https://github.com/apache/arrow-rs/pull/1700
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #1700: Fix StructArrayReader handling nested lists (#1651)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1700:
URL: https://github.com/apache/arrow-rs/pull/1700#discussion_r872415235
##########
parquet/src/arrow/array_reader/map_array.rs:
##########
@@ -154,15 +150,15 @@ impl ArrayReader for MapArrayReader {
}
fn get_def_levels(&self) -> Option<&[i16]> {
- self.def_level_buffer
- .as_ref()
- .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()
Review Comment:
Drive by fix, part of #1699
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #1700: Fix StructArrayReader handling nested lists (#1651)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1700:
URL: https://github.com/apache/arrow-rs/pull/1700#discussion_r873684689
##########
parquet/src/arrow/array_reader.rs:
##########
@@ -1564,12 +1541,74 @@ mod tests {
.collect::<Vec<bool>>()
);
assert_eq!(
- Some(vec![0, 1, 1, 1, 1].as_slice()),
+ Some(vec![0, 1, 2, 3, 1].as_slice()),
struct_array_reader.get_def_levels()
);
assert_eq!(
Some(vec![0, 1, 1, 1, 1].as_slice()),
struct_array_reader.get_rep_levels()
);
}
+
+ #[test]
+ fn test_struct_array_reader_list() {
+ use arrow::datatypes::Int32Type;
+ // [
+ // {foo: [1, 2, null],
+ // {foo: []},
+ // {foo: null},
+ // null,
+ // ]
+
+ let expected_l =
+ Arc::new(ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
+ Some(vec![Some(1), Some(2), None]),
+ Some(vec![]),
+ None,
+ None,
+ ]));
+
+ let nulls = Buffer::from([0b00000111]);
Review Comment:
Will rename to validity, it is an arrow null mask... I agree the naming is perpetually confusing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #1700: Fix StructArrayReader handling nested lists (#1651)
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1700:
URL: https://github.com/apache/arrow-rs/pull/1700#discussion_r873989019
##########
parquet/src/arrow/array_reader.rs:
##########
@@ -1564,12 +1541,74 @@ mod tests {
.collect::<Vec<bool>>()
);
assert_eq!(
- Some(vec![0, 1, 1, 1, 1].as_slice()),
+ Some(vec![0, 1, 2, 3, 1].as_slice()),
struct_array_reader.get_def_levels()
);
assert_eq!(
Some(vec![0, 1, 1, 1, 1].as_slice()),
struct_array_reader.get_rep_levels()
);
}
+
+ #[test]
+ fn test_struct_array_reader_list() {
+ use arrow::datatypes::Int32Type;
+ // [
+ // {foo: [1, 2, null],
+ // {foo: []},
+ // {foo: null},
+ // null,
+ // ]
+
+ let expected_l =
+ Arc::new(ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
+ Some(vec![Some(1), Some(2), None]),
+ Some(vec![]),
+ None,
+ None,
+ ]));
+
+ let nulls = Buffer::from([0b00000111]);
Review Comment:
aaah!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org