Fix `MapArrayReader` (#2484) (#1699) (#1561) #2500

tustvold · 2022-08-18T13:35:07Z

Which issue does this PR close?

Closes #2484
Closes #1699
Closes #1561

Rationale for this change

The ListArrayReader logic is better tested, more complete and can be used to read MapArrays

What changes are included in this PR?

Replaces the logic in MapArrayReader with ListArrayReader

Are there any user-facing changes?

No

tustvold · 2022-08-18T13:35:30Z

parquet/src/arrow/array_reader/map_array.rs

+    use arrow::record_batch::RecordBatch;
+    use bytes::Bytes;
+
+    #[test]


Credit to @d-willis for this test

tustvold · 2022-08-18T13:38:41Z

parquet/src/arrow/array_reader/map_array.rs

+            ArrowType::Map(element, _) => match element.data_type() {
+                ArrowType::Struct(fields) if fields.len() == 2 => {
+                    // The inner map field must always non-nullable (#1697)
+                    assert!(!element.is_nullable(), "map struct cannot be nullable");


As described in #1697 the encoding of DataType::Map permits greater nullability than the type can actually contain

#1697 is a question -- so is the solution "we will assume that the inner struct can not be nullable until we have an existence proof to the contrary?"

I'd say it is more, if this level is nullable the schema is inconsistent as there is no way to represent that in parquet 😅 The schema inference logic will never generate this - https://github.com/apache/arrow-rs/blob/master/parquet/src/arrow/schema/complex.rs#L350

maybe worth a comment there

tvincent2 · 2022-08-18T15:36:38Z

Hi!
You just made my week with this PR!

It's quite a coincidence, but we've been hit by this very problem and spent some time on it. We created a small project to show how to reproduce our issue and just found #2484 as we were about to open our issue.
We can confirm that we can no longer reproduce our issue when using this PR branch.

Thanks a lot!

Cheers,
Thomas

d-willis · 2022-08-18T16:57:06Z

Giving this a test against our code now. Hopefully will have run by tomorrow...
I can confirm that the reproducer test now works correctly locally.

alamb

Thank you @tustvold - I think this looks really nice (it is awesome to fix bugs by deleting code).

I think the test should be updated to assert that nullness on the read array was correct, but otherwise this PR looks good to go to me.

The feedback from @tvincent2 (thanks for that btw 👋 ) also makes a compelling case for the benefits of this PR. 👍

cc @nevi-me and @viirya

alamb · 2022-08-18T16:53:54Z

parquet/src/arrow/array_reader/map_array.rs

+            ArrowType::Map(element, _) => match element.data_type() {
+                ArrowType::Struct(fields) if fields.len() == 2 => {
+                    // The inner map field must always non-nullable (#1697)
+                    assert!(!element.is_nullable(), "map struct cannot be nullable");


#1697 is a question -- so is the solution "we will assume that the inner struct can not be nullable until we have an existence proof to the contrary?"

alamb · 2022-08-18T16:55:40Z

parquet/src/arrow/array_reader/map_array.rs

-            map_def_level: rep_level,
-            map_rep_level: def_level,
-        }
+        let struct_def_level = match nullable {


Perhaps we could document what nullable means in this context as part of a docstring

alamb · 2022-08-18T16:56:32Z

parquet/src/arrow/array_reader/map_array.rs

+                    assert!(!element.is_nullable(), "map struct cannot be nullable");
+                    element
+                }
+                _ => unreachable!("expected struct with two fields"),


eventually it might be a nicer UX to return an error rather than panic'ing here (e.g. MapArrayReader::try_new()) but that would also change the API

This isn't a public API so I could change it, but that also makes me less inclined to do so. It is a bug in the builder if you run into this

makes sense 👍

alamb · 2022-08-18T16:57:36Z

parquet/src/arrow/array_reader/map_array.rs

+        let data = array.data().clone();
+        let builder = data.into_builder().data_type(self.data_type.clone());
+        Ok(Arc::new(MapArray::from(unsafe {
+            builder.build_unchecked()


This unsafe is OK because the inner StructArrayReader already validated the contents of the structs, and MapArrayReader::new() already validated the DataType?

Pretty much, will add a doc comment

alamb · 2022-08-18T16:59:51Z

parquet/src/arrow/array_reader/map_array.rs

-    map_def_level: i16,
-    #[allow(unused)]
-    map_rep_level: i16,
+    reader: ListArrayReader<i32>,


calling this inner might me more idiomatic

alamb · 2022-08-18T17:02:05Z

parquet/src/arrow/array_reader/map_array.rs

+        for maybe_record_batch in record_batch_reader {
+            let record_batch = maybe_record_batch.expect("Getting current batch");
+            let col = record_batch.column(0);
+            let map_entry = array::as_map_array(col).value(2);


Shouldn't this test also validate that the first two entries are null? Something like

assert!(!array::as_map_array(col).is_valid(0)); assert!(!array::as_map_array(col).is_valid(1));

The test was originally written to test my very specific failure condition.

Possibly a worthwhile addition though.

Yeah, sorry I probably should have phrased that as "I suggest this test also validate the first two entries are null"

ursabot · 2022-08-18T17:52:24Z

Benchmark runs are scheduled for baseline = f3afdd2 and contender = c34f07f. c34f07f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

alamb · 2022-08-18T17:52:42Z

parquet/src/arrow/array_reader/map_array.rs

@@ -193,6 +201,8 @@ mod tests {
        for maybe_record_batch in record_batch_reader {
            let record_batch = maybe_record_batch.expect("Getting current batch");
            let col = record_batch.column(0);
+            assert!(col.is_null(0));


d-willis · 2022-08-19T12:42:29Z

Thanks for the quick turn around on this!

Fix MapArrayReader (apache#2484) (apache#1699) (apache#1561)

9768bd9

tustvold commented Aug 18, 2022

View reviewed changes

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

tustvold commented Aug 18, 2022

View reviewed changes

Fix comments

137c5ed

alamb changed the title ~~Fix MapArrayReader (#2484) (#1699) (#1561)~~ Fix MapArrayReader (#2484) (#1699) (#1561) Aug 18, 2022

alamb approved these changes Aug 18, 2022

View reviewed changes

Review feedback

f4168a9

tustvold merged commit c34f07f into apache:master Aug 18, 2022

alamb reviewed Aug 18, 2022

View reviewed changes

tustvold mentioned this pull request Aug 19, 2022

Support parquet map type in arrow IO #396

Closed

tustvold mentioned this pull request Oct 20, 2022

[Rust] Parquet data source does not support complex types apache/datafusion#83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `MapArrayReader` (#2484) (#1699) (#1561) #2500

Fix `MapArrayReader` (#2484) (#1699) (#1561) #2500

tustvold commented Aug 18, 2022

tustvold Aug 18, 2022

tustvold Aug 18, 2022

alamb Aug 18, 2022

tustvold Aug 18, 2022

alamb Aug 18, 2022

tvincent2 commented Aug 18, 2022

d-willis commented Aug 18, 2022

alamb left a comment

alamb Aug 18, 2022

alamb Aug 18, 2022

alamb Aug 18, 2022

tustvold Aug 18, 2022

alamb Aug 18, 2022

alamb Aug 18, 2022

tustvold Aug 18, 2022

alamb Aug 18, 2022

alamb Aug 18, 2022

d-willis Aug 18, 2022

d-willis Aug 18, 2022

alamb Aug 18, 2022

ursabot commented Aug 18, 2022

alamb Aug 18, 2022

d-willis commented Aug 19, 2022

Fix MapArrayReader (#2484) (#1699) (#1561) #2500

Fix MapArrayReader (#2484) (#1699) (#1561) #2500

Conversation

tustvold commented Aug 18, 2022

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

tvincent2 commented Aug 18, 2022

d-willis commented Aug 18, 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

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

ursabot commented Aug 18, 2022

Choose a reason for hiding this comment

d-willis commented Aug 19, 2022

Fix `MapArrayReader` (#2484) (#1699) (#1561) #2500

Fix `MapArrayReader` (#2484) (#1699) (#1561) #2500