Test and fix for bug writing a null list. #1477

novemberkilo · 2022-03-23T04:45:52Z

Which issue does this PR close?

Closes #1036

What changes are included in this PR?

A test and a minor fix. I'm not sure if the fix is a bit naive or not.

This was blocked by #1399 but now that the reader support is in for null lists, I am resurrecting this work.

novemberkilo · 2022-03-23T04:53:42Z

Possibly of interest @viirya @sunchao @tustvold @alamb ?

codecov-commenter · 2022-03-23T05:12:54Z

Codecov Report

Merging #1477 (1948d4f) into master (d384836) will increase coverage by 0.00%.
The diff coverage is 96.15%.

@@           Coverage Diff           @@
##           master    #1477   +/-   ##
=======================================
  Coverage   82.70%   82.70%           
=======================================
  Files         187      187           
  Lines       54208    54233   +25     
=======================================
+ Hits        44832    44855   +23     
- Misses       9376     9378    +2

Impacted Files	Coverage Δ
parquet/src/arrow/levels.rs	`84.88% <91.66%> (+0.21%)`	⬆️
parquet/src/arrow/arrow_writer.rs	`97.60% <100.00%> (+0.04%)`	⬆️
parquet_derive/src/parquet_field.rs	`65.98% <0.00%> (-0.23%)`	⬇️
arrow/src/array/transform/mod.rs	`86.35% <0.00%> (-0.23%)`	⬇️
arrow/src/datatypes/datatype.rs	`66.80% <0.00%> (+0.39%)`	⬆️

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 d384836...1948d4f. Read the comment docs.

parquet/src/arrow/arrow_writer.rs

parquet/src/arrow/levels.rs

viirya · 2022-03-23T05:27:57Z

parquet/src/arrow/levels.rs

@@ -757,7 +757,10 @@ impl LevelInfo {

    /// Given a level's information, calculate the offsets required to index an array correctly.
    pub(crate) fn filter_array_indices(&self) -> Vec<usize> {


Should we have a test case for list array for filter_array_indices ?

Sounds sensible thanks @viirya I will try adding one.

This function only makes sense for writing leaf arrays, and so I worry that whilst this may fix the case of a List of nulls, it will behave erratically if given a list of non-nulls. I'll see if I can work out why this method is getting called at all

Edit: Well this sure looks suspect - https://github.com/apache/arrow-rs/blob/master/parquet/src/arrow/levels.rs#L203

Ok I have a fix but whilst writing this I've found what appears to be yet another bug 😞

Unfortunately it is EOD here, but I'll pick this back up first thing tomorrow (GMT) and see if I can't get a PR to fix both this bug and the one I've just discovered

novemberkilo · 2022-03-23T05:57:27Z

parquet/src/arrow/levels.rs

@@ -1822,4 +1821,22 @@ mod tests {
        // filter_array_indices should return the same indices in this case.
        assert_eq!(level1.filter_array_indices(), level2.filter_array_indices());
    }
+
+    #[test]
+    fn test_filter_indices_for_lists() {


I don't understand the machinery of levels so I don't know if I have made a nonsense test here or not. It passes ... but I don't know if that means anything yet. Does it seem sensible? // @viirya

I'm not familiar with definition/repetition level computation. I will take a look at the algorithm.

That's my fault on the complexity here. Let's see if I can interpret the LevelInfo.

A nullable list will have 3 levels if its child array is also nullable.

So with the def levels:

0: null list value null

1: empty list []

2: list with null [null]

3: list with value [1]

Here's a comment earlier in this file (https://github.com/apache/arrow-rs/blob/master/parquet/src/arrow/levels.rs#L402-L407). In this case, l1 = 0, l2 = 1, l3 = 2. With l3, the actual emptiness of the value (null or 1 above) gets determined by the child.

If the child is a non-null array, then def = 3 isn't needed as it'd always be true, so we'd only have def.max() == 2. With the same pattern, if the list is non-null, then we only need 2 levels.

0: empty list

1: list with value

The repetition levels let us know how nested the list is, so 0 is the start of a new list item, and 1 is a continuation.

If we have repetition like: [0, 1, 2, 1, 0, 2] plus its definitions, we can both construct the offsets (meaning that LevelInfo is inefficient as we don't really need its array_offsets) as follows:

Offsets = every value that's 0 is a new record, but if there's a 0, we have to check if the definition has an empty value for that record.

So we start with 2 list values:

[0, 1, 2, 1] [0, 2]

Then we expand further to:

[ [0], [1, 2], [1] ] -> 3 nested values in the list slot [ [0, 2] ] -> 1 nested value in the list slot

Using the offsets on your test (with reps shown):

1. [0, 1, 2] (rep: [0, 1, 1]) 2. [] (rep: [0]) 3. [3, 4, 5] (rep: [0, 1, 1])

So in this case filter_array_indices is giving us the indices that have values, hence the [0, 1, 2, 3, 4, 5] result.

So maybe a good approach for null list values is to add definition = 2. I'd expect whichever index that's got a def of 2 or less to be excluded.

I'll be able to have a look at this PR tomorrow morning GMT, so I can tweak it or answer questions in detail then if you don't mind @novemberkilo

FWIW if you're interested in understanding what these repetition levels actually are there is a good explanation of how the parquet record shredding works here.

I also attempted to explain to myself what this function is doing which may be helpful - https://github.com/apache/arrow-rs/pull/1185/files#r785357233

Okay. I tried to study the repetition/definition level algorithm a little bit. Seems this issue is not at filter_array_indices, although the error is bumped out there.

Seems we should not have List level info there for this case.

It comes from https://github.com/apache/arrow-rs/blob/master/parquet/src/arrow/levels.rs#L203, we reuse the level info for the list. Looks suspect as @tustvold mentioned.

novemberkilo · 2022-03-24T09:34:42Z

Thanks folks for engaging with this. Looks like the attention and discussion is flushing out the correct fix. I am relatively new to this project and while this is excellent at helping me learn, I wonder if this issue is best left to @tustvold and @viirya to tackle correctly. Happy to close this PR in favour of their work.

tustvold · 2022-03-24T11:13:25Z

Ok I've filed #1481 which I think should fix the underlying bugs at play here. Thanks again @novemberkilo for reporting and drawing attention to this issue 👍

FWIW if you're interested in helping flesh out the rather limited support for nested types I would be more than happy to assist with filing tickets, reviews, etc... There is definitely a large amount of improvement that could be made in this area, what we support, what we test, etc... and I would be more than happy to assist where possible, just let me know 😄

novemberkilo · 2022-03-24T22:52:44Z

Closing in favour of #1481

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

Test and fix for bug writing a null list.

308e79e

novemberkilo force-pushed the topic/null-list-writer-fix branch from 19d242e to 308e79e Compare March 23, 2022 04:52

viirya reviewed Mar 23, 2022

View reviewed changes

parquet/src/arrow/arrow_writer.rs Outdated Show resolved Hide resolved

viirya reviewed Mar 23, 2022

View reviewed changes

parquet/src/arrow/levels.rs Outdated Show resolved Hide resolved

Remove cruft.

290f615

viirya reviewed Mar 23, 2022

View reviewed changes

Test filter_array_indices when level type is list.

1948d4f

novemberkilo commented Mar 23, 2022

View reviewed changes

novemberkilo closed this Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test and fix for bug writing a null list. #1477

Test and fix for bug writing a null list. #1477

novemberkilo commented Mar 23, 2022

novemberkilo commented Mar 23, 2022

codecov-commenter commented Mar 23, 2022 •

edited

Loading

viirya Mar 23, 2022

novemberkilo Mar 23, 2022

tustvold Mar 23, 2022 •

edited

Loading

tustvold Mar 23, 2022 •

edited

Loading

novemberkilo Mar 23, 2022

viirya Mar 23, 2022

nevi-me Mar 23, 2022 •

edited

Loading

tustvold Mar 23, 2022

viirya Mar 24, 2022

novemberkilo commented Mar 24, 2022

tustvold commented Mar 24, 2022

novemberkilo commented Mar 24, 2022

		@@ -757,7 +757,10 @@ impl LevelInfo {

		/// Given a level's information, calculate the offsets required to index an array correctly.
		pub(crate) fn filter_array_indices(&self) -> Vec<usize> {

Test and fix for bug writing a null list. #1477

Test and fix for bug writing a null list. #1477

Conversation

novemberkilo commented Mar 23, 2022

Which issue does this PR close?

What changes are included in this PR?

novemberkilo commented Mar 23, 2022

codecov-commenter commented Mar 23, 2022 • edited Loading

Codecov Report

viirya Mar 23, 2022

Choose a reason for hiding this comment

novemberkilo Mar 23, 2022

Choose a reason for hiding this comment

tustvold Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

tustvold Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

novemberkilo Mar 23, 2022

Choose a reason for hiding this comment

viirya Mar 23, 2022

Choose a reason for hiding this comment

nevi-me Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

tustvold Mar 23, 2022

Choose a reason for hiding this comment

viirya Mar 24, 2022

Choose a reason for hiding this comment

novemberkilo commented Mar 24, 2022

tustvold commented Mar 24, 2022

novemberkilo commented Mar 24, 2022

codecov-commenter commented Mar 23, 2022 •

edited

Loading

tustvold Mar 23, 2022 •

edited

Loading

tustvold Mar 23, 2022 •

edited

Loading

nevi-me Mar 23, 2022 •

edited

Loading