Add support for nested list arrays from parquet to arrow arrays (#993) #1588

tustvold · 2022-04-18T23:02:17Z

Which issue does this PR close?

Closes #993.

Closes #720

Rationale for this change

Adds support for reading nested lists from parquet.

What changes are included in this PR?

Reworks the ListArrayReader to handle nested repetition levels.

Some drive by cleanup of ArrayReaderBuilder to error on encountering non-spec-compliant data

Are there any user-facing changes?

No

tustvold · 2022-05-04T11:50:42Z

parquet/src/arrow/array_reader/builder.rs

-        let key_reader = {
-            let mut key_context = new_context.clone();
-            key_context.def_level += 1;
-            key_context.path.append(vec![map_key.name().to_string()]);


This is a drive-by fix, the context is the context of the parent, not the value being dispatched. This would result in map_key appearing twice

tustvold · 2022-05-04T11:53:03Z

parquet/src/arrow/array_reader/builder.rs


+        // If the list can contain nulls


I recommend reading this with whitespace disabled https://github.com/apache/arrow-rs/pull/1588/files?w=1

tustvold · 2022-05-04T15:38:54Z

parquet/src/arrow/array_reader/list_array.rs

+        )
+    }
+
+    fn test_nested_list<OffsetSize: OffsetSizeTrait>() {


I think this wins the prize of the most 🤯 test I've ever written

tustvold · 2022-05-04T15:39:57Z

parquet/src/arrow/array_reader/builder.rs

-                    new_context.def_level += 1;
-                    new_context.rep_level += 1;
-                    false
+                    return Err(ArrowError(format!(


This just moves the error to earlier, there is no point continuing if this is not supported anyway 😁

tustvold · 2022-05-04T15:41:10Z

parquet/src/arrow/array_reader/builder.rs

-                ArrowType::List(_)
-                | ArrowType::FixedSizeList(_, _)
-                | ArrowType::Dictionary(_, _) => Err(ArrowError(format!(
-                    "reading List({:?}) into arrow not supported yet",


This is the error that would previously be returned on a nested list

codecov-commenter · 2022-05-04T16:17:50Z

Codecov Report

Merging #1588 (15b8144) into master (8f24c45) will increase coverage by 0.13%.
The diff coverage is 82.16%.

@@            Coverage Diff             @@
##           master    #1588      +/-   ##
==========================================
+ Coverage   83.02%   83.16%   +0.13%     
==========================================
  Files         193      193              
  Lines       55612    56005     +393     
==========================================
+ Hits        46174    46574     +400     
+ Misses       9438     9431       -7

Impacted Files	Coverage Δ
parquet/src/schema/visitor.rs	`68.00% <0.00%> (+0.89%)`	⬆️
parquet/src/arrow/array_reader/builder.rs	`68.97% <59.77%> (+0.96%)`	⬆️
parquet/src/arrow/array_reader/test_util.rs	`81.08% <80.00%> (-2.26%)`	⬇️
parquet/src/arrow/array_reader/list_array.rs	`93.28% <93.22%> (+5.31%)`	⬆️
parquet/src/arrow/array_reader.rs	`89.76% <100.00%> (-0.07%)`	⬇️
arrow/src/array/array.rs	`89.67% <0.00%> (-3.04%)`	⬇️
parquet_derive/src/parquet_field.rs	`65.98% <0.00%> (-0.23%)`	⬇️
arrow/src/ipc/reader.rs	`88.80% <0.00%> (-0.23%)`	⬇️
parquet/src/encodings/encoding.rs	`93.37% <0.00%> (-0.19%)`	⬇️
arrow/src/compute/kernels/take.rs	`95.27% <0.00%> (-0.14%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f24c45...15b8144. Read the comment docs.

tustvold · 2022-05-04T16:38:09Z

parquet/src/arrow/array_reader/list_array.rs

+        let mut skipped = 0;
+
+        // Builder used to construct the filtered child data, skipping empty lists and nulls
+        let mut child_data_builder = MutableArrayData::new(


I switched to using this over the take kernel as I think it makes it easier to follow what is going on, it should also be significantly faster where nulls and empty lists are rare

tustvold · 2022-05-04T17:21:09Z

I've confirmed this can read nested_schema.parquet from @andrei-ionescu on apache/datafusion#1383.

I've also confirmed this can read https://github.com/Igosuki/arrow2/blob/main/part-00000-b4749aa1-94e4-4ddb-bab2-954c4d3a290f.c000.snappy.parquet and https://github.com/chauhanVritul/sampleparquet/blob/main/part-00000-8e5acb24-eb4e-491c-8c85-88799f25d1f0-c000.snappy.parquet provided by @Igosuki on #720

I've done a manual inspection that the output is the same as duckdb, and it looks good

andrei-ionescu · 2022-05-04T17:22:53Z

Thank you @tustvold

alamb

I can't say I am a deep expert in this area or this part of the codebase, but I did give the PR a careful "software engineering" review, with careful study of the tests. LGTM 👍

FWIW whitespace blind diff https://github.com/apache/arrow-rs/pull/1588/files?w=1 helped in my review

cc @nevi-me and @sunchao in case you would like to review

alamb · 2022-05-06T18:16:46Z

parquet/src/schema/visitor.rs

    /// item type.
    ///
    /// To fully understand this algorithm, please refer to
    /// [parquet doc](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md).
+    ///
+    /// For example, a standard list type looks like:


nice document addition 👍

parquet/src/arrow/array_reader/list_array.rs

alamb · 2022-05-06T18:32:07Z

parquet/src/arrow/array_reader/list_array.rs

+            .build()
+            .unwrap();
+
+        let offsets = to_offsets::<OffsetSize>(vec![0, 4, 4, 4, 5]);


Would it be valuable to have a sublist that has more than 1 item?

It appears that this test only has null, [] or a single element [x] list. I may be misunderstanding the intent of the test or what is possible in nested lists

There is a [1, null], can probably add more though

Done in 05c2311

alamb · 2022-05-06T18:32:55Z

parquet/src/arrow/array_reader/list_array.rs

+            true,
+        );
+
+        let actual = l1.next_batch(1024).unwrap();


Would it also make sense to test with l1.next_batch(2) (aka something that doesn't decode the entire array in one go?)

Done in 05c2311, required adding functionality to InMemoryArrayReader to actually respect it

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

tustvold · 2022-05-08T17:27:45Z

parquet/src/arrow/array_reader.rs

@@ -1532,15 +1532,15 @@ mod tests {
            ArrowType::Int32,
            array_1.clone(),
            Some(vec![0, 1, 2, 3, 1]),
-            Some(vec![1, 1, 1, 1, 1]),
+            Some(vec![0, 1, 1, 1, 1]),


Drive by fix, this set of levels is impossible, as the first rep level must be 0. Nothing cares as this test doesn't actually decode the implied list, but 🤷

viirya

Looked at the changes, especially on ListArrayReader. I'm not Parquet expert too, but the change looks reasonable when looking at with the added comments. 👍

viirya · 2022-05-08T18:02:14Z

parquet/src/arrow/array_reader/list_array.rs

+    /// The definition level at which this list is not null
+    def_level: i16,
+    /// The repetition level that corresponds to a new value in this array
+    rep_level: i16,
+    /// If this list is nullable
+    nullable: bool,


The comment looks clear. 👍

viirya · 2022-05-08T18:09:08Z

parquet/src/schema/visitor.rs

    /// item type.
    ///
    /// To fully understand this algorithm, please refer to
    /// [parquet doc](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md).
+    ///
+    /// For example, a standard list type looks like:


nice document addition 👍

github-actions bot added the parquet Changes to the parquet crate label Apr 18, 2022

tustvold force-pushed the nested-arrays branch from 6ba6299 to fbffd2f Compare April 18, 2022 23:03

Add support for nested list arrays (apache#993)

d3537c8

tustvold force-pushed the nested-arrays branch from fbffd2f to d3537c8 Compare May 4, 2022 11:45

tustvold commented May 4, 2022

View reviewed changes

More tests

bf32cb6

tustvold commented May 4, 2022

View reviewed changes

Minor cleanup

70c16c3

Filter nulls

e9acb22

tustvold changed the title ~~WIP: Add support for nested list arrays (#993)~~ Add support for nested list arrays (#993) May 4, 2022

tustvold marked this pull request as ready for review May 4, 2022 16:35

tustvold commented May 4, 2022

View reviewed changes

tustvold added 2 commits May 4, 2022 17:39

Update comments

b3ca78e

Fix doc

1d00e7c

Fix clippy

fdae1d5

alamb changed the title ~~Add support for nested list arrays (#993)~~ Add support for nested list arrays from parquet to arrow arrays (#993) May 5, 2022

alamb approved these changes May 6, 2022

View reviewed changes

tustvold and others added 4 commits May 8, 2022 15:39

Apply suggestions from code review

1b99c49

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

More tests

05c2311

Add sanity check to ListArrayReader

f250ba7

Fix test_struct_array_reader

15b8144

tustvold commented May 8, 2022

View reviewed changes

viirya approved these changes May 8, 2022

View reviewed changes

tustvold merged commit edd96c0 into apache:master May 9, 2022

tustvold added a commit to tustvold/arrow-rs that referenced this pull request May 9, 2022

Fix logical merge conflict in apache#1588

9615cf2

tustvold mentioned this pull request May 9, 2022

Fix logical merge conflict in #1588 #1678

Merged

alamb pushed a commit that referenced this pull request May 9, 2022

Fix logical merge conflict in #1588 (#1678)

e02869a

tustvold mentioned this pull request May 26, 2022

Use BitChunkIterator in Parquet Writer Level Computation #1757

Open

tustvold mentioned this pull request Oct 20, 2022

Reading nested parquet files results in index out of bounds apache/datafusion#1383

Closed

This was referenced Oct 28, 2022

Parquet list reader not reading correct element lengths #518

Closed

ParquetFileArrowReader panicks when trying to read nested list #163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nested list arrays from parquet to arrow arrays (#993) #1588

Add support for nested list arrays from parquet to arrow arrays (#993) #1588

tustvold commented Apr 18, 2022 •

edited

Loading

tustvold May 4, 2022

tustvold May 4, 2022

tustvold May 4, 2022

tustvold May 4, 2022

tustvold May 4, 2022

codecov-commenter commented May 4, 2022 •

edited

Loading

tustvold May 4, 2022

tustvold commented May 4, 2022

andrei-ionescu commented May 4, 2022

alamb left a comment

alamb May 6, 2022

viirya May 8, 2022

alamb May 6, 2022

tustvold May 6, 2022

tustvold May 8, 2022

alamb May 6, 2022

tustvold May 8, 2022

tustvold May 8, 2022

viirya left a comment

viirya May 8, 2022

viirya May 8, 2022

Add support for nested list arrays from parquet to arrow arrays (#993) #1588

Add support for nested list arrays from parquet to arrow arrays (#993) #1588

Conversation

tustvold commented Apr 18, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tustvold commented May 4, 2022

andrei-ionescu commented May 4, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Apr 18, 2022 •

edited

Loading

codecov-commenter commented May 4, 2022 •

edited

Loading