ARROW-7842: [Rust] [Parquet] implement array_reader for list type columns #6770

mcassels · 2020-03-30T23:31:18Z

Nested lists are not yet supported. PR for new test file for datafusion test: apache/parquet-testing#11

github-actions · 2020-03-30T23:31:29Z

https://issues.apache.org/jira/browse/ARROW-7842

mcassels · 2020-04-22T23:00:29Z

@andygrove @nevi-me do you have any thoughts on this?

andygrove · 2020-04-22T23:35:00Z

rust/datafusion/src/logicalplan.rs

-        \n    TableScan: employee.csv projection=Some([0, 3])";
+                        \n  Selection: #state Eq Utf8(\"CO\")\
+                        \n    TableScan: employee.csv projection=Some([0, 3])";



Are these formatting changes from running rustfmt?

andygrove · 2020-04-22T23:35:49Z

rust/datafusion/src/utils.rs

+        let list = $column
+            .as_any()
+            .downcast_ref::<array::ListArray>()
+            .unwrap()


expect would be better than unwrap. Using a Result would be better than expect.

andygrove · 2020-04-22T23:36:47Z

rust/datafusion/src/utils.rs

+            Ok(values) => values,
+            _ => {
+                return Err(ExecutionError::ExecutionError(format!(
+                    "Unsupported {:?} type for repl.",


What does repl mean here? Maybe this error could be a bit more descriptive?

andygrove · 2020-04-22T23:37:40Z

rust/datafusion/src/utils.rs

        DataType::Time64(unit) if *unit == TimeUnit::Nanosecond => {
            make_string!(array::Time64NanosecondArray, column, row)
        }
+        DataType::List(_) => make_string_from_list!(column, row),


Does it matter what data type the list is? Can lists of any type be converted into strings?

This recursively calls array_value_to_string on the array items, so an unsupported type should give the appropriate error on the recursive call

andygrove

Sorry for the late review. I wasn't aware of this PR until you mentioned me. My email filters didn't match the title ... this looks great overall. I left a few comments and would like to see some feedbackfrom someone more familiar with the parquet crate. I've requested some other reviewers take a look. Thanks for the contribution!

nevi-me

LGTM

nevi-me · 2020-05-03T10:09:51Z

rust/parquet/src/arrow/array_reader.rs

+        ArrowType::Time64(ArrowTimeUnit::Nanosecond) => {
+            build_empty_list_array_with_primitive_items!(ArrowTime64NanosecondType)
+        }
+        ArrowType::Duration(ArrowTimeUnit::Second) => {


Are durations supported in parquet?

It looks like the PrimitiveArrayReader implementation supports Duration

That's probably incorrect, but we can clarify/fix the behaviour of the readers at a later stage.

maxburke · 2020-05-05T03:34:28Z

Is there anything else that needs to be taken care of in order to have this merged?

nevi-me · 2020-05-05T09:37:34Z

Is there anything else that needs to be taken care of in order to have this merged?

We need to wait for apache/parquet-testing#11 to be merged, then update the submodule so that the tests pass.

@sunchao @paddyhoran may you please also have a look at this when you get a chance. I have some slight concerns around Arrow <-> Parquet format support, but I'm comfortable that we'll be able to pick up any required changes when we progress with the Arrow to Parquet writer.

sunchao

Sorry again for the late review. Have being out of touch for some time and is taking time to get back to the latest state of Arrow reader in parquet-rs. Left some comments but will probably add more later.

sunchao · 2020-05-22T18:07:39Z

rust/parquet/src/arrow/array_reader.rs


    /// Build array reader for list type.
-    /// Currently this is not supported.
    fn visit_list_with_item(


Is this being called anywhere?

Yeah, it's called here: https://github.com/apache/arrow/blob/master/rust/parquet/src/schema/visitor.rs#L51

sunchao · 2020-05-22T18:09:43Z

rust/parquet/src/arrow/array_reader.rs

+    }};
+}
+
+fn build_empty_list_array(item_type: ArrowType) -> Result<ArrayRef> {


The way this is implemented isn't ideal as it has to do pattern matching on every possible type. Is there a way to simplify this? For instance, every ArrowPrimitiveType already has a DataType as its field.

I don't know whether there's a way to do the association in the opposite direction. In this case we want to go from a DataType to an ArrowPrimitiveType. I found that pattern matching is currently used in other similar situations like https://github.com/apache/arrow/blob/master/rust/arrow/src/json/reader.rs#L433 for example

sunchao · 2020-05-22T18:10:16Z

rust/parquet/src/arrow/array_reader.rs

+        &self.data_type
+    }
+
+    fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {


Does this handle nested lists?

No, this PR doesn't handle nested lists. At the end of visit_list_with_item there's a check on the item type and it gives an error if the List item type is List https://github.com/apache/arrow/pull/6770/files#diff-fca053527b552a9f2abb1e280e570cb6R1197

Ok. It will be good if we have a path to get there. Can also reference the C++ impl. Also could you add some comments in this to indicate that nested lists are not supported?

I've just added a couple comments -- on the ArrayReader implementation and on visit_list_with_item

rust/datafusion/src/logicalplan.rs

rust/datafusion/src/execution/physical_plan/expressions.rs

sunchao

@mcassels thanks for your patience and I added more comments on this PR.

rust/arrow/src/ipc/convert.rs

sunchao · 2020-05-28T23:55:56Z

rust/parquet/src/arrow/array_reader.rs

+            && (rep_levels.len() == next_batch_array.len()))
+        {
+            return Err(ArrowError(
+                "Expected item_reader def_level and rep_level arrays to have the same length as batch array".to_string(),


nit: long line

sunchao · 2020-05-28T23:58:47Z

rust/parquet/src/arrow/array_reader.rs

+        if next_batch_array.len() == 0 {
+            return build_empty_list_array(item_type);
+        }
+        let def_levels = self.item_reader.get_def_levels().unwrap();


propagate possible errors instead of unwrap?

sunchao · 2020-05-29T00:19:39Z

rust/parquet/src/arrow/array_reader.rs

+        let mut new_context = context.clone();
+
+        let list_child = &list_type.get_fields()[0];
+        let item_child = &list_child.get_fields()[0];


This doesn't look correct. I think we should use the _item_type passed in for item type. List type in Parquet can be represented with several different kinds of schema as mentioned here. This method already helped to setup the list item type for you.

Yes, that makes more sense. I'm not sure why I did it that way before

The failing test made me realize why I had it that way. Currently item_type is passed as &Type to visit_list_with_item while list_type is passed as Rc<Type>. Because item_type is not passed as Rc<Type>, if item_type is used then when the list items are visited by PrimitiveArrayReader, they will not satisfy the self.is_included check, because that checks using the reference.

I've now changed the signature of visit_list_with_item to pass item_type as Rc<Type> to fix this issue.

sunchao · 2020-05-29T00:22:24Z

rust/parquet/src/arrow/array_reader.rs

+        &self.data_type
+    }
+
+    fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {


Ok. It will be good if we have a path to get there. Can also reference the C++ impl. Also could you add some comments in this to indicate that nested lists are not supported?

sunchao · 2020-05-29T00:23:39Z

rust/parquet/src/arrow/array_reader.rs

+
+        // null list has def_level = 0
+        // empty list has def_level = 1
+        // null item in a list has def_level = 2


I think we also need to check if the list item is required or optional right? the list itself can also be either optional or requried.

Yeah it seems that this comment is not correct... I think it would be correct just in the case where both the list and the list item are optional.

However, much of the logic in this comment is not actually used: Rep and def levels are compared only with 0. Do you see any flaws in the def and rep level logic that comes after this comment? If the actual logic is ok I'll just remove the comment.

sunchao · 2020-05-29T00:40:40Z

rust/parquet/src/arrow/array_reader.rs

+        }
+        let batch_values = match null_list_indices.len() {
+            0 => next_batch_array.clone(),
+            _ => remove_indices(next_batch_array.clone(), item_type, null_list_indices)?,


Hmm why you need to process the batch array and add them through a builder? why cannot just directly use the array in the result ListArray and set up validity bits and offsets only?

As I understand it, the null values that represent a null list rather than a null list item need to not be present in the child data of the final ListArray. I don't like that this uses the builder either, but I wasn't sure how else to create that child data array that has specific indices excluded... I might be misunderstanding something though. Do you have any suggestions?

Not sure if I understand you correctly. I think a null list is represented with the combination of valid bit array and offset array, and should have no presence in the value array (example here). Therefore, the input array can just be passed intact to the ListArray as value array.

It would also be useful to refer the C++ impl.

emkornfield · 2020-08-27T03:51:25Z

@nevi-me did your other PRs supercede this?

nevi-me · 2020-09-16T02:06:21Z

I'll pick this up in the coming 2 weeks. @maxburke @mcassels I might need write access to the urbanlogiq fork, but I can ping you privately and ask for access temporarily until we get this merged.

nevi-me · 2020-09-26T01:02:26Z

This has sadly fallen behind significantly from the main branch. I've spent a few hours trying to rebase, but because it looks like the PR contained changes for the below, it's a bit difficult to follow.

filter kernel for lists
contains kernel (also can't tell if it was introduced then removed in subsequent commits, but looks like that)
the parquet list reader

I got as far as getting everything except the filter kernel working, but I suspect I'll be spending a few more hours dealing with merge conflicts; so I'm throwing in the towel for now. I'll try a different strategy of reimplementing the parquet changes on the main branch separately.

@yordan-pavlov would you have capacity to help with filter kernels for lists? We would use @mcassels work, but convert it to use the FilterContext that you introduced (IIRC).

nevi-me · 2020-09-26T01:04:33Z

@nevi-me did your other PRs supercede this?

I hadn't seen this @emkornfield. No, I've only been focusing on the writer, so I'll still want to merge this into the branch currently called master.
I've also noticed that we could split the PR into 2 or 3, so I'll try do that as I've spent a few hours trying to rebase; and I'll have to throw away that time :(

yordan-pavlov · 2020-09-26T10:36:11Z

@nevi-me I am happy to help but I couldn't see any changes to the filter kernel in this PR; have I misunderstood your message?

nevi-me · 2020-09-26T11:19:19Z

@nevi-me I am happy to help but I couldn't see any changes to the filter kernel in this PR; have I misunderstood your message?

Apologies, this PR came from a fork that UrbanLogiq maintain, so I think they had worked on list filters for their purposes, and then initially included those commits in this PR, but eventually reversed them.

I think it's still good to incorporate those changes, as the relevant JIRA has been open for a while (ARROW-5350).

I've extracted the changes and pushed them to my fork at nevi-me@4c11f46. You'll see that I commented out the filter on lists, as I couldn't figure out how to make them work with FilterContext as mentioned.

There's already some tests, so looks like a solid base to start from. You can copy the changes from my commit, or push into my ARROW-5350 branch; either's fine with me.

yordan-pavlov · 2020-09-28T17:33:53Z

thanks @nevi-me , I understand now, will have a look this week

mcassels · 2020-09-29T22:53:26Z

@nevi-me I am happy to help but I couldn't see any changes to the filter kernel in this PR; have I misunderstood your message?

Apologies, this PR came from a fork that UrbanLogiq maintain, so I think they had worked on list filters for their purposes, and then initially included those commits in this PR, but eventually reversed them.

I think it's still good to incorporate those changes, as the relevant JIRA has been open for a while (ARROW-5350).

I've extracted the changes and pushed them to my fork at nevi-me@4c11f46. You'll see that I commented out the filter on lists, as I couldn't figure out how to make them work with FilterContext as mentioned.

There's already some tests, so looks like a solid base to start from. You can copy the changes from my commit, or push into my ARROW-5350 branch; either's fine with me.

Yes we had worked on list filters as well but intended to make that a separate PR. But as you can see unfortunately we got busy and fell behind main :/

velvia · 2020-09-29T23:59:44Z

@nevi-me @yordan-pavlov Hi so I joined UrbanLogiq recently and am helping to maintain this stuff. Turns out this has become important to fix and merge for us too and we need to bring our branch up to date. Let us know best way to go forward, but one of us is happy to start porting this change to a newer updated branch and let everyone have a look, and push a formal Arrow PR as well.

nevi-me · 2020-09-30T00:05:46Z

@nevi-me @yordan-pavlov Hi so I joined UrbanLogiq recently and am helping to maintain this stuff. Turns out this has become important to fix and merge for us too and we need to bring our branch up to date. Let us know best way to go forward, but one of us is happy to start porting this change to a newer updated branch and let everyone have a look, and push a formal Arrow PR as well.

I've already ported the changes to a separate branch, and I'm currently going through @sunchao's review. I ran out of time over the weekend, but I might be able to complete the changes in the coming week.

I know it creates a dependency on me, but given the hours I've already spent on this; I'd rather still continue and address the review, as I might be able to merge the new PR immediately if I don't have questions for anyone.

velvia · 2020-09-30T17:42:20Z

I know it creates a dependency on me, but given the hours I've already spent on this; I'd rather still continue and address the review, as I might be able to merge the new PR immediately if I don't have questions for anyone.

@nevi-me that sounds great, looking forward to it! There is another feature we might look to push from our branch sometime. :)

nevi-me · 2020-10-12T21:51:57Z

Closed in favour of #8449

This is a port of apache#6770 to the parquet-writer branch. We'll have more of a chance to test this reader, and ensure that we can roundtrip on list types.

This is a port of apache#6770 to the parquet-writer branch. We'll have more of a chance to test this reader, and ensure that we can roundtrip on list types. [Rust] [Parquet] LargeListArray support and why I think the tests are still failing (#6) * Support reading LargeListArrays by making ListArrayReader generic over OffsetSize * Update comment to match the actual values in this test; probably copy-paste * Document why I think the test setup isn't quite right disable list writer tests They're failing because of incorrect def/rep. Will be addressed separately

This is a port of #6770 to the parquet-writer branch. We'll have more of a chance to test this reader,and ensure that we can roundtrip on list types. Closes #8449 from nevi-me/ARROW-7842-cherry Authored-by: Neville Dipale <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

mcassels mentioned this pull request Mar 31, 2020

ARROW-7842 add tiny parquet with int and utf8 list columns apache/parquet-testing#11

Merged

Morgan Cassels added 7 commits March 31, 2020 13:11

implement list columns

5acff2c

cleanup and fix tests

3fb8fb5

more cleanup

48ee841

remove unused import

dd4b109

more cleanup

f9411f1

remove unnecessary test

f4e70fd

rebase and cleanup repl list printing

8c8efa1

mcassels force-pushed the read_list_type_columns branch from dc2c861 to 8c8efa1 Compare March 31, 2020 20:13

wesm force-pushed the master branch from 5fe5b88 to aa55967 Compare April 19, 2020 22:46

kszucs force-pushed the master branch from 1b71ca7 to 5093b80 Compare April 20, 2020 19:21

andygrove changed the title ~~ARROW-7842 [Parquet][Rust] implement array_reader for list type columns~~ ARROW-7842: [Rust] [Parquet] implement array_reader for list type columns Apr 22, 2020

andygrove added the Component: Rust label Apr 22, 2020

andygrove requested review from nevi-me and paddyhoran April 22, 2020 23:31

andygrove reviewed Apr 22, 2020

View reviewed changes

andygrove requested a review from sunchao April 22, 2020 23:38

andygrove reviewed Apr 22, 2020

View reviewed changes

sunchao added the Component: Parquet label Apr 24, 2020

address comment

feb72d3

nevi-me approved these changes May 3, 2020

View reviewed changes

mcassels added 2 commits May 3, 2020 18:50

Merge branch 'master' into read_list_type_columns

57132e6

cargo fmt

6f90636

sunchao reviewed May 22, 2020

View reviewed changes

sunchao requested changes May 29, 2020

View reviewed changes

Morgan Cassels and others added 3 commits June 8, 2020 20:51

code review

70d40db

Merge branch 'master' into read_list_type_columns

1de5b9b

pass item type as Rc to visit_list_with_item

e24df6e

nevi-me mentioned this pull request Sep 26, 2020

ARROW-10103: [Rust] Add contains kernel #8280

Closed

nevi-me mentioned this pull request Oct 12, 2020

ARROW-7842: [Rust] [Parquet] Arrow list reader #8449

Closed

nevi-me closed this Oct 12, 2020

asfimport mentioned this pull request Dec 27, 2020

[Rust] [Parquet] Implement array reader for list type #24070

Closed

ARROW-7842: [Rust] [Parquet] implement array_reader for list type columns #6770

ARROW-7842: [Rust] [Parquet] implement array_reader for list type columns #6770

Uh oh!

Conversation

mcassels commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 30, 2020

Uh oh!

mcassels commented Apr 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcassels May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxburke commented May 5, 2020

Uh oh!

nevi-me commented May 5, 2020

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcassels commented Mar 30, 2020 •

edited

Loading

mcassels May 4, 2020 •

edited

Loading

nevi-me commented Sep 16, 2020 •

edited

Loading

velvia commented Sep 29, 2020 •

edited

Loading