Fix Parquet reader for null lists #1448

viirya · 2022-03-15T20:39:31Z

Which issue does this PR close?

Closes #1399.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

viirya · 2022-03-15T20:40:37Z

parquet/src/arrow/arrow_reader.rs

+    #[test]
+    fn test_read_null_list() {
+        let testdata = arrow::util::test_util::parquet_test_data();
+        let path = format!("{}/null_list.parquet", testdata);


I downloaded the test parquet file from @novemberkilo's parquet-testing fork to test it locally. Not sure how we can put the file there?

You could point to my parquet-testing fork like I have done here novemberkilo@e5952ae#diff-fe7afb5c9c916e521401d3fcfb4277d5071798c3baf83baf11d6071742823584

Okay, I think it works for CI purpose in the PR. Not sure if we can merge into master like that. Anyway, let me point to your fork first to test it. Thanks.

@novemberkilo could you open a PR against parquet-testing and add your test file there?

yea, I think it is good idea.

@sunchao apache/parquet-testing#23

@viirya fwiw I did something like:

delete the parquet-testing submodule

git submodule add -b topic/null-list-reader [email protected]:novemberkilo/parquet-testing

Oh, I see. Thanks. I added the file manually to test it before.

And seems updating .gitmodules doesn't work for CI here. We may need to wait for the test file being merged in parquet-testing.

I will have a play with creating a test that generates a parquet file, so that we can get this PR in without waiting for parquet-testing. I will also file a ticket to start a discussion on a faster way to get parquet test files checked in, without relying on an upstream repo

Thanks @tustvold. The parquet file was merged. It'd be great to get testing parquet test files checked in faster.

viirya

I'm not sure if I understand correctly Parquet's behavior for this case. It looks like next should return None. That's what I do in this fix.

cc @sunchao

viirya · 2022-03-15T20:49:43Z

parquet/src/arrow/array_reader.rs

@@ -924,7 +927,7 @@ impl<OffsetSize: OffsetSizeTrait> ArrayReader for ListArrayReader<OffsetSize> {
            && (rep_levels.len() == next_batch_array.len()))
        {
            return Err(ArrowError(
-                "Expected item_reader def_levels and rep_levels to be same length as batch".to_string(),
+                format!("Expected item_reader def_levels {} and rep_levels {} to be same length as batch {}", def_levels.len(), rep_levels.len(), next_batch_array.len()),


Purely for debugging purpose. I feel it is clear.

novemberkilo · 2022-03-16T07:56:30Z

Thanks for taking this on @viirya -- I'm following along with interest :)

tustvold

This looks good to me, thanks - I will see if I can't create some minimal test cases so that we can get this one in (and to check I fully understand the problems)

tustvold · 2022-03-21T12:45:13Z

parquet/src/arrow/array_reader.rs

@@ -1984,12 +1990,14 @@ impl<'a> ArrayReaderBuilder {
                    field = self.arrow_schema.field_with_name(part).ok();
                } else if let Some(f) = field {
                    if let ArrowType::Struct(fields) = f.data_type() {
-                        field = fields.iter().find(|f| f.name() == part)
+                        field = fields.iter().find(|f| f.name() == part);


Perhaps a match block might be clearer?

tustvold · 2022-03-21T13:03:26Z

parquet/src/arrow/array_reader.rs

@@ -885,6 +885,9 @@ fn remove_indices(
                Ok(Arc::new(StructArray::from((new_columns, valid.finish()))))
            }
        }
+        ArrowType::Null => {


Probably beyond the scope of this PR, but it occurs to me that this remove_indices function is basically an inverse of the arrow take kernel. I wonder if we could flip the construction of null_list_indices to be instead the non-null positions - and just use a regular take kernel?

I might have a play with this 🤔

Yea, seems so.

tustvold · 2022-03-21T13:09:11Z

parquet/src/arrow/array_reader.rs

@@ -1984,12 +1990,14 @@ impl<'a> ArrayReaderBuilder {
                    field = self.arrow_schema.field_with_name(part).ok();


Not part of this PR, but I wonder why this doesn't just initialize field to be self.arrow_schema.field_with_name(parts[1]).ok() and then do skip(2) given it has already done the length check above...

tustvold · 2022-03-21T13:17:15Z

parquet/src/arrow/array_reader.rs

@@ -1984,12 +1990,14 @@ impl<'a> ArrayReaderBuilder {
                    field = self.arrow_schema.field_with_name(part).ok();
                } else if let Some(f) = field {
                    if let ArrowType::Struct(fields) = f.data_type() {
-                        field = fields.iter().find(|f| f.name() == part)
+                        field = fields.iter().find(|f| f.name() == part);
+                    } else if let ArrowType::List(list_field) = f.data_type() {


If I'm not mistaken this change is necessary to support a StructArray containing a ListArray in general, without being specific to nulls?

Yea, this is not specific to nulls.

tustvold · 2022-03-21T13:20:01Z

parquet/src/arrow/arrow_reader.rs

+    #[test]
+    fn test_read_null_list() {
+        let testdata = arrow::util::test_util::parquet_test_data();
+        let path = format!("{}/null_list.parquet", testdata);


I will have a play with creating a test that generates a parquet file, so that we can get this PR in without waiting for parquet-testing. I will also file a ticket to start a discussion on a faster way to get parquet test files checked in, without relying on an upstream repo

tustvold · 2022-03-21T15:04:39Z

parquet/src/arrow/arrow_reader.rs

+            .expect("Failed to read into array!");
+
+        let batch = record_batch_reader.next();
+        assert!(batch.is_none());


I don't think this is correct, locally I ran

> duckdb.query(f"select count(*) from 'null_list.parquet'").fetchall() [(1,)] > duckdb.query(f"select * from 'null_list.parquet'").fetchall() [(None,)]

Which would imply that the data contains a single row, with a single null value

Perhaps it should be

let batch = record_batch_reader.next().unwrap().unwrap(); assert_eq!(batch.num_rows(), 1); assert_eq!(batch.num_columns(), 1); assert_eq!(batch.column(0).len(), 1); let list = batch.column(0).as_any().downcast_ref::<ListArray>().unwrap(); assert_eq!(list.len(), 1); assert_eq!(list.is_valid(0), true); let val = list.value(0); assert_eq!(val.len(), 0);

tustvold · 2022-03-21T15:20:52Z

parquet/src/arrow/array_reader.rs

@@ -964,7 +967,10 @@ impl<OffsetSize: OffsetSizeTrait> ArrayReader for ListArrayReader<OffsetSize> {
                cur_offset += OffsetSize::one();
            }
        });
-        offsets.push(cur_offset);
+
+        if !cur_offset.is_zero() {


I'm not sure this is correct, I think it will convert an array containing no non-null values, into an empty array?

codecov-commenter · 2022-03-22T06:55:53Z

Codecov Report

Merging #1448 (acd8d47) into master (717216f) will increase coverage by 0.04%.
The diff coverage is 82.75%.

@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
+ Coverage   82.67%   82.72%   +0.04%     
==========================================
  Files         185      187       +2     
  Lines       53866    54186     +320     
==========================================
+ Hits        44535    44824     +289     
- Misses       9331     9362      +31

Impacted Files	Coverage Δ
parquet/src/arrow/array_reader.rs	`78.72% <64.28%> (+0.45%)`	⬆️
parquet/src/arrow/arrow_reader.rs	`93.79% <100.00%> (+0.22%)`	⬆️
arrow/src/array/data.rs	`82.95% <0.00%> (-0.21%)`	⬇️
arrow/src/array/array_list.rs	`95.52% <0.00%> (ø)`
integration-testing/src/lib.rs	`0.00% <0.00%> (ø)`
arrow/src/compute/kernels/window.rs	`100.00% <0.00%> (ø)`
parquet_derive/src/parquet_field.rs	`65.98% <0.00%> (ø)`
arrow/src/array/transform/union.rs	`75.00% <0.00%> (ø)`
arrow/src/array/transform/fixed_size_list.rs	`73.68% <0.00%> (ø)`
arrow/src/array/transform/mod.rs	`86.46% <0.00%> (+0.03%)`	⬆️
... and 6 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 717216f...acd8d47. Read the comment docs.

tustvold

Thank you for this, unless anybody else chimes in with a review I'll get this merged this afternoon 👍

alamb · 2022-03-22T10:09:04Z

Merging while we get the permissions for @tustvold sorted out. Thanks all!

viirya · 2022-03-22T16:10:08Z

Thank you @alamb @tustvold @novemberkilo @sunchao !

Fix Parquet reader for null list

46aa8ef

github-actions bot added the parquet Changes to the parquet crate label Mar 15, 2022

viirya commented Mar 15, 2022

View reviewed changes

Test on forked parquet-testing

ee121d3

novemberkilo mentioned this pull request Mar 17, 2022

Add empty file with list data apache/parquet-testing#23

Merged

tustvold reviewed Mar 21, 2022

View reviewed changes

For review comments

acd8d47

Fix clippy

d9b6d0e

tustvold approved these changes Mar 22, 2022

View reviewed changes

alamb merged commit e778c10 into apache:master Mar 22, 2022

This was referenced Mar 24, 2022

Fix reading/writing nested null arrays (#1480) (#1036) (#1399) #1481

Merged

Cleanup: Use Arrow take kernel Within parquet ListArrayReader #1482

Closed

alamb changed the title ~~Fix Parquet reader for null list~~ Fix Parquet reader for null lists Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Parquet reader for null lists #1448

Fix Parquet reader for null lists #1448

viirya commented Mar 15, 2022

viirya Mar 15, 2022 •

edited

Loading

novemberkilo Mar 16, 2022

viirya Mar 16, 2022

sunchao Mar 16, 2022

viirya Mar 16, 2022

novemberkilo Mar 17, 2022

novemberkilo Mar 17, 2022

viirya Mar 17, 2022

tustvold Mar 21, 2022

viirya Mar 21, 2022

viirya left a comment •

edited

Loading

viirya Mar 15, 2022

novemberkilo commented Mar 16, 2022

tustvold left a comment

tustvold Mar 21, 2022

tustvold Mar 21, 2022

viirya Mar 22, 2022

tustvold Mar 21, 2022

tustvold Mar 21, 2022

viirya Mar 22, 2022

tustvold Mar 21, 2022

tustvold Mar 21, 2022 •

edited

Loading

tustvold Mar 21, 2022

codecov-commenter commented Mar 22, 2022

tustvold left a comment

alamb commented Mar 22, 2022

viirya commented Mar 22, 2022

		@@ -1984,12 +1990,14 @@ impl<'a> ArrayReaderBuilder {
		field = self.arrow_schema.field_with_name(part).ok();

Fix Parquet reader for null lists #1448

Fix Parquet reader for null lists #1448

Conversation

viirya commented Mar 15, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

viirya Mar 15, 2022 • 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

viirya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberkilo commented Mar 16, 2022

tustvold 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

tustvold Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 22, 2022

Codecov Report

tustvold left a comment

Choose a reason for hiding this comment

alamb commented Mar 22, 2022

viirya commented Mar 22, 2022

viirya Mar 15, 2022 •

edited

Loading

viirya left a comment •

edited

Loading

tustvold Mar 21, 2022 •

edited

Loading