Parquet Reader/writer for fixed-size list arrays #4267

sydduckworth · 2023-05-23T14:51:22Z

Which issue does this PR close?

What changes are included in this PR?

Parquet writer support for fixed-size lists
Parquet reader support for fixed-size lists

Reader and writer both include unit tests.
I also sanity-checked the reader and writer against pyarrow (although I wasn't able to verify writing lists with nulls against pyarrow due to this outstanding issue).

Are there any user-facing changes?

None other than the added type support

The implementation still needs tests. The implementation uses a new `write_fixed_size_list` method instead of `write_list`. This is done to avoid the overhead of needlessly calculating list offsets.

The implementation still needs tests.

Fixed bugs in implementation found via tests.

Writer now emits the correct definition levels for empty lists. Added empty list unit test.

Reader now handles empty list definition levels correctly. Added empty list unit test.

…to-parquet

tustvold

Thank you for this, I've left some comments. I think the level encoding is currently not quite correct, as it doesn't correctly delimit the row starts.

I think the test coverage of this is pretty good, but I think the following would highlight where the current implementation is not quite right (or show that it is and I am mistaken 😄 ):

A test reading a RecordBatch containing both a FixedSizeList column and a primitive column, this will test the record delimiting logic works correctly - as written I think it will error with length mismatch as it won't read enough data from the FixedSizeList child
A test reading a parquet file with skip_arrow_metadata, so it will infer the list as a regular List - this can then be used to verify that the data was encoded correctly - and would read correctly with a reader that doesn't understand FixedSizeList

parquet/src/arrow/array_reader/fixed_size_list_array.rs

tustvold · 2023-05-23T17:41:08Z

parquet/src/arrow/array_reader/fixed_size_list_array.rs

+                        child_data_builder.extend_nulls(self.fixed_size);
+
+                        if let Some(validity) = validity.as_mut() {
+                            // Valid if empty list


I think an empty list should be an error?

It was unclear to me whether the Arrow spec allows a fixed-size list of length zero. It seemed like it's supported, since pyarrow will read and write zero length lists and a zero length fixed-size list encodes to the same def/rep levels as an empty dynamically sized list.

However, I'd be happy to change the implementation to emit an error if that behavior isn't desired!

Aah this is a good point, a zero-sized fixed size list is certainly a peculiar construction, but not technically invalid I don't think

parquet/src/arrow/array_reader/fixed_size_list_array.rs

parquet/src/arrow/arrow_writer/levels.rs

tustvold · 2023-05-23T17:55:20Z

parquet/src/arrow/arrow_writer/levels.rs

+                    // Mark the start of each list in the child array
+                    for i in 0..num_items {
+                        let idx = start + i * fixed_size;
+                        rep_levels[idx] = ctx.rep_level - 1;


This doesn't seem to be correct, only the first element should be ctx.rep_level - 1, the rest should be ctx.rep_level

Correct me if I'm wrong, but my understanding is that the child elements' write methods should handle writing the leaf repetition levels So the fixed-size list writer should only need to set the repetition level to encode the start of each row.

Aah, I misread this the first time. I believe the logic isn't quite correct where you have a repeated child within the FixedSizeList.

In particular it is making the erroneous assumption that there will be fixed_size leaves.

I wonder if we could make write_list have the signature

fn write_list<I, O: OffsetSizeTrait>( &mut self, ranges: I, nulls: Option<&NullBuffer>, values: &dyn Array, ) where I: IntoIterator<Item=(O, O)>

And use this to allow sharing the same logic 🤔

parquet/src/arrow/arrow_writer/levels.rs

tustvold · 2023-05-23T18:01:21Z

parquet/src/arrow/arrow_writer/levels.rs

+                let mut start_idx = None;
+                for idx in range.clone() {
+                    if nulls.is_valid(idx) {
+                        // Start a run of valid rows if not already inside of one


I don't think coercing the rows like this is correct, as the repetition of each row must be different

Shouldn't the repetition level inside each row be handled by the child elements? My implementation seems to match the same logic used by write_struct, but maybe I'm missing something?

You are quite correct, it has been a while since I've had to touch this code 😄

tustvold · 2023-05-23T18:02:13Z

parquet/src/arrow/arrow_writer/levels.rs

+
+        let expected_level = LevelInfo {
+            def_levels: Some(vec![0, 0, 3, 3]),
+            rep_levels: Some(vec![0, 0, 0, 1]),


Suggested change

rep_levels: Some(vec![0, 0, 0, 1]),

rep_levels: Some(vec![0, 1, 0, 1]),

As written it is not correct

I believe that the original is correct since the levels are generated by builder.write(&a, 1..4).

So the levels should correspond to the 1st, 2nd, and 3rd rows:

def 0, rep 0 (row 1, null)

def 0, rep 0 (row 2, null)

def 3, rep 0 (row 3, child 0)

def 3, rep 1 (row 3, child 1)

Is that not right?

sydduckworth · 2023-05-23T20:18:22Z

@tustvold thanks for the feedback!

I implemented most of your corrections, and I added the two tests you suggested.
I also updated the test that writes a list of structs so that the structs contain 2 fields instead of 1, to try to catch any potential errors in the def/rep level encoding.

Following those changes, I believe the rep/def level writer logic is correct as-is, but it's possible I'm missing something.

tustvold

Thank you for this, I think this is mostly good to go. I don't think it correctly encodes levels for FixedSizeList with repeated children, but I'm also happy for that to be fixed as a follow on PR if you would prefer. Stellar work on this 💪

tustvold · 2023-05-24T11:23:16Z

parquet/src/arrow/arrow_writer/levels.rs

+                    // Mark the start of each list in the child array
+                    for i in 0..num_items {
+                        let idx = start + i * fixed_size;
+                        rep_levels[idx] = ctx.rep_level - 1;


Aah, I misread this the first time. I believe the logic isn't quite correct where you have a repeated child within the FixedSizeList.

In particular it is making the erroneous assumption that there will be fixed_size leaves.

I wonder if we could make write_list have the signature

fn write_list<I, O: OffsetSizeTrait>( &mut self, ranges: I, nulls: Option<&NullBuffer>, values: &dyn Array, ) where I: IntoIterator<Item=(O, O)>

And use this to allow sharing the same logic 🤔

tustvold · 2023-05-24T11:23:39Z

parquet/src/arrow/arrow_writer/levels.rs

+                let mut start_idx = None;
+                for idx in range.clone() {
+                    if nulls.is_valid(idx) {
+                        // Start a run of valid rows if not already inside of one


You are quite correct, it has been a while since I've had to touch this code 😄

tustvold · 2023-05-24T11:25:20Z

parquet/src/arrow/array_reader/fixed_size_list_array.rs

+                        child_data_builder.extend_nulls(self.fixed_size);
+
+                        if let Some(validity) = validity.as_mut() {
+                            // Valid if empty list


Aah this is a good point, a zero-sized fixed size list is certainly a peculiar construction, but not technically invalid I don't think

parquet/src/arrow/array_reader/fixed_size_list_array.rs

Converted the check to return an error instead of panicking.

Writer now correctly handles child arrays with variable row length. Added new unit test to verify the new behavior is correct.

Test verifies that reader handles child arrays with variable length correctly.

sydduckworth · 2023-05-24T14:23:10Z

@tustvold I've finally understood the encoding logic error you were pointing out 😅.
I added tests for both reader and writer to verify correct encoding/decoding of fixed-length lists containing variable-length lists, and the incorrect behavior on the write side should now be fixed.

tustvold

Looks good to me, thank you

* Initial implementation for writing fixed-size lists to Parquet. The implementation still needs tests. The implementation uses a new `write_fixed_size_list` method instead of `write_list`. This is done to avoid the overhead of needlessly calculating list offsets. * Initial implementation for reading fixed-size lists from Parquet. The implementation still needs tests. * Added tests for fixed-size list writer. Fixed bugs in implementation found via tests. * Added tests for fixed-size list reader. Fixed bugs in implementation found via tests. * Added correct behavior for writing empty fixed-length lists. Writer now emits the correct definition levels for empty lists. Added empty list unit test. * Added correct behavior for reading empty fixed-length lists. Reader now handles empty list definition levels correctly. Added empty list unit test. * Fixed linter warnings. * Added license header to fixed_size_list_array.rs * Added fixed-size list reader tests from PR review. * Added fixed-size reader row length sanity checks. * Simplified fixed-size list case in LevelInfoBuilder constructor. * Removed dynamic dispatch inside fixed-length list writer. * Expanded list of structs test for fixed-size list writer. * Reverted expected levels in fixed-size list writer test. * Fixed linter warnings. * Updated list size check in fixed-size list reader. Converted the check to return an error instead of panicking. * Small tweak to row length check in fixed-size list reader. * Fixed bug in fixed-size list level encoding. Writer now correctly handles child arrays with variable row length. Added new unit test to verify the new behavior is correct. * Added fixed-size list reader test. Test verifies that reader handles child arrays with variable length correctly.

Dexter Duckworth added 7 commits May 22, 2023 14:07

Initial implementation for writing fixed-size lists to Parquet.

cb49982

The implementation still needs tests. The implementation uses a new `write_fixed_size_list` method instead of `write_list`. This is done to avoid the overhead of needlessly calculating list offsets.

Initial implementation for reading fixed-size lists from Parquet.

2571533

The implementation still needs tests.

Added tests for fixed-size list writer.

b4782f2

Fixed bugs in implementation found via tests.

Added tests for fixed-size list reader.

8e834ce

Fixed bugs in implementation found via tests.

Added correct behavior for writing empty fixed-length lists.

567c6ca

Writer now emits the correct definition levels for empty lists. Added empty list unit test.

Added correct behavior for reading empty fixed-length lists.

c73ca80

Reader now handles empty list definition levels correctly. Added empty list unit test.

Fixed linter warnings.

007f158

github-actions bot added the parquet Changes to the parquet crate label May 23, 2023

Merge branch 'apache:master' into 4214-support-writing-fixedsizelist-…

04ad0f1

…to-parquet

tustvold reviewed May 23, 2023

View reviewed changes

Dexter Duckworth added 7 commits May 23, 2023 14:25

Added license header to fixed_size_list_array.rs

2d533e6

Added fixed-size list reader tests from PR review.

4222c52

Added fixed-size reader row length sanity checks.

52f403c

Simplified fixed-size list case in LevelInfoBuilder constructor.

e169b8d

Removed dynamic dispatch inside fixed-length list writer.

f18783b

Expanded list of structs test for fixed-size list writer.

205f63c

Reverted expected levels in fixed-size list writer test.

727c3be

Fixed linter warnings.

002ea35

tustvold approved these changes May 24, 2023

View reviewed changes

Dexter Duckworth added 4 commits May 24, 2023 09:11

Updated list size check in fixed-size list reader.

1ecf67d

Converted the check to return an error instead of panicking.

Small tweak to row length check in fixed-size list reader.

6254384

Fixed bug in fixed-size list level encoding.

3876a16

Writer now correctly handles child arrays with variable row length. Added new unit test to verify the new behavior is correct.

Added fixed-size list reader test.

a483044

Test verifies that reader handles child arrays with variable length correctly.

tustvold approved these changes May 24, 2023

View reviewed changes

tustvold merged commit 741244d into apache:master May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parquet Reader/writer for fixed-size list arrays #4267

Parquet Reader/writer for fixed-size list arrays #4267

sydduckworth commented May 23, 2023

tustvold left a comment •

edited

Loading

tustvold May 23, 2023

sydduckworth May 23, 2023

tustvold May 24, 2023

tustvold May 23, 2023

sydduckworth May 23, 2023

tustvold May 24, 2023

tustvold May 23, 2023

sydduckworth May 23, 2023

tustvold May 24, 2023

tustvold May 23, 2023

sydduckworth May 23, 2023

sydduckworth commented May 23, 2023

tustvold left a comment •

edited

Loading

tustvold May 24, 2023

tustvold May 24, 2023

tustvold May 24, 2023

sydduckworth commented May 24, 2023

tustvold left a comment

	rep_levels: Some(vec![0, 0, 0, 1]),
	rep_levels: Some(vec![0, 1, 0, 1]),

Parquet Reader/writer for fixed-size list arrays #4267

Parquet Reader/writer for fixed-size list arrays #4267

Conversation

sydduckworth commented May 23, 2023

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydduckworth commented May 23, 2023

tustvold left a comment • edited Loading

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

sydduckworth commented May 24, 2023

tustvold left a comment

Choose a reason for hiding this comment

tustvold left a comment •

edited

Loading

tustvold left a comment •

edited

Loading