Minimal MapArray support #491

nevi-me · 2021-06-22T16:19:40Z

Which issue does this PR close?

Related #395 .

Rationale for this change

To add support for the map array :)

What changes are included in this PR?

This adds minimal support for arrow::array::MapArray and Parquet map support.

Includes

arrow::datatype::Map(Box<Field>, bool /* keys_sorted */ )
arrow::array::MapArray
a really minimal arrow::array::MapArrayBuilder. Note, this API was just to make progress, so more work is needed
arrow::ipc::{reader, writer} support
arrow::json::reader support
Equality support
Arrow schema <> Parquet schema
Arrow <> Parquet reader
Arrow <> Parquet writer
- Primitive maps

Future work

See #538

Are there any user-facing changes?

Yes, new data type, enum, etc. Builds shall break

codecov-commenter · 2021-07-03T08:32:24Z

Codecov Report

Merging #491 (ef6944e) into master (2b2ec7e) will increase coverage by 0.03%.
The diff coverage is 80.54%.

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   82.48%   82.52%   +0.03%     
==========================================
  Files         167      168       +1     
  Lines       46450    47227     +777     
==========================================
+ Hits        38315    38973     +658     
- Misses       8135     8254     +119

Impacted Files	Coverage Δ
arrow/src/array/data.rs	`73.76% <0.00%> (+0.15%)`	⬆️
arrow/src/ipc/writer.rs	`86.22% <ø> (ø)`
arrow/src/array/equal/mod.rs	`93.45% <25.00%> (-0.48%)`	⬇️
arrow/src/array/equal_json.rs	`88.69% <33.33%> (-2.52%)`	⬇️
arrow/src/datatypes/field.rs	`51.36% <45.00%> (-0.47%)`	⬇️
arrow/src/datatypes/datatype.rs	`66.08% <50.00%> (-0.74%)`	⬇️
parquet/src/arrow/array_reader.rs	`77.70% <77.67%> (+0.25%)`	⬆️
arrow/src/array/builder.rs	`85.97% <81.44%> (-0.29%)`	⬇️
arrow/src/util/integration_util.rs	`70.10% <81.81%> (+0.54%)`	⬆️
parquet/src/arrow/schema.rs	`87.35% <81.88%> (-1.18%)`	⬇️
... and 18 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 2b2ec7e...ef6944e. Read the comment docs.

nevi-me · 2021-07-11T12:29:41Z

@alamb @jorgecarleitao this is now ready for review

alamb · 2021-07-12T20:02:13Z

@nevi-me I plan to review this over the next few days

alamb

I read this whole PR -- and I think it is well written, well commented, and well tested and think it is good to merge in. I had some minor feedback but nothing I think would prevent merging.

Epic work @nevi-me 🏅

Regarding the "experimental API" -- I wonder if we could avoid it. For example, if we just merged this PR after arrow 5.0.0 is released (I hope to cut the branch / make an RC tomorrow) we can freely change the APIs on the master branch until arrow 6.0.0 is cut and not worry about backwards compatibility

arrow/src/array/array.rs

arrow/src/array/array_map.rs

alamb · 2021-07-13T20:15:27Z

arrow/src/array/array_map.rs

+            .add_buffer(entry_offsets)
+            .add_child_data(entry_struct.data().clone())
+            .build();
+        let map_array = MapArray::from(map_data);


up to here, this method seems almost identical to create_from_buffers -- perhaps it could be reused?

I've made some slight differences between the two functions to test null values on the one map

arrow/Cargo.toml

arrow/src/array/array_map.rs

alamb · 2021-07-13T20:24:52Z

arrow/src/array/transform/structure.rs

-                mutable.child_data.iter_mut().for_each(|child| {
-                    child.extend(
-                        index,
-                        array.offset() + start,


why not use the array.offset()? I don't understand this change

This comes from the struct slice fix, I had rebased it into the map support so it wouldn't block my progress. I've rebased now that the PR is merged.

arrow/src/json/reader.rs

parquet/src/arrow/arrow_reader.rs

alamb · 2021-07-13T21:03:49Z

parquet/src/arrow/arrow_reader.rs

+            .expect("Failed to read into array!");
+
+        for batch in record_batch_reader {
+            batch.unwrap();


It seems like it would be valuable to check the contents of the record batches that were read out (in addition to checking they are not errors)

This is a bit tricky @alamb, because there's no output data to compare what's read to. The parquet test data that's used by parquet-mr, parquet-cpp and parquet-rs don't have equivalent JSON files like other integration tests.

So, the next best thing has been to read the files, and confirm that the reader doesn't error out.

alamb · 2021-07-13T21:04:44Z

parquet/src/arrow/levels.rs

+                        map_field.data_type()
+                    );
+                }
+            }
            DataType::FixedSizeList(_, _) => unimplemented!(),


I wonder if it would be useful to test in this module as well

nevi-me · 2021-07-14T14:46:14Z

@alamb I'm working on addressing your feedback, will update this PR in a few hours

This commit adds the MapArray and MapBuilder. The interfaces are however incomplete at this stage.

A map is a list with some specific rules, so for equality it is the same as a list

Avoids the generic case of list > struct > [ley, value], which adds overhead

nevi-me · 2021-07-29T20:43:22Z

@alamb I've addressed your review, PTAL

alamb

I looked at the last two commits and they look great @nevi-me !

houqp · 2021-07-31T20:07:35Z

thank you @nevi-me !

nevi-me · 2021-09-01T16:03:04Z

I read this whole PR -- and I think it is well written, well commented, and well tested and think it is good to merge in. I had some minor feedback but nothing I think would prevent merging.

Epic work @nevi-me 🏅

Regarding the "experimental API" -- I wonder if we could avoid it. For example, if we just merged this PR after arrow 5.0.0 is released (I hope to cut the branch / make an RC tomorrow) we can freely change the APIs on the master branch until arrow 6.0.0 is cut and not worry about backwards compatibility

@alamb, do you think it would be possible to include this in the next 5.x release? I forgot that we never added this PR to active releases, so it hasn't been released yet.

houqp · 2021-09-07T20:24:11Z

If this introduces breaking API changes, I think the current policy is we will have to wait for 6.x?

alamb · 2021-09-09T21:53:39Z

As @houqp mentions, I think this one can not be included into 5.x without breaking compatibility.

Specifically, the introduction of DataType::Map will cause existing code that relies on that enum to fail to compile. Here is an example of that happening in a WIP PR in datafusion - apache/datafusion#984 (apache/datafusion#984 (comment))

nevi-me added the api-change Changes to the arrow API label Jun 22, 2021

github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Jun 22, 2021

nevi-me force-pushed the map-support branch 2 times, most recently from 82fec61 to 2d7ddd5 Compare June 23, 2021 18:37

nevi-me force-pushed the map-support branch 2 times, most recently from 8be09ae to 83d8406 Compare July 3, 2021 08:18

nevi-me force-pushed the map-support branch from ed527dd to b5efa36 Compare July 11, 2021 06:01

nevi-me mentioned this pull request Jul 11, 2021

make slice work for nested types #389

Merged

nevi-me force-pushed the map-support branch from b5efa36 to 85cda50 Compare July 11, 2021 12:01

nevi-me changed the title ~~[DRAFT] Minimal MapArray support~~ Minimal MapArray support Jul 11, 2021

nevi-me mentioned this pull request Jul 11, 2021

DataType::Map tracking issue #538

Closed

5 tasks

nevi-me requested review from alamb, jorgecarleitao and sunchao July 11, 2021 12:29

nevi-me marked this pull request as ready for review July 11, 2021 12:29

alamb approved these changes Jul 13, 2021

View reviewed changes

nevi-me force-pushed the map-support branch from f52d13e to 35a5b06 Compare July 14, 2021 18:53

nevi-me force-pushed the map-support branch from 35a5b06 to 20ddfde Compare July 22, 2021 07:51

nevi-me mentioned this pull request Jul 22, 2021

Slicing list<struct<...>> doesn't pass the list offsets to struct children #594

Closed

nevi-me added 6 commits July 29, 2021 22:36

add DataType::Map to datatypes

123b4d1

barebones MapArray and MapBuilder

301c478

This commit adds the MapArray and MapBuilder. The interfaces are however incomplete at this stage.

minimal IPC read and write

731d3a2

barebones MapArray (missed)

cdad831

add equality for map, relying on list

881b063

A map is a list with some specific rules, so for equality it is the same as a list

json reader for MapArray

261f611

nevi-me added 9 commits July 29, 2021 22:36

add schema roundtrip

9b066a2

read and write maps from/to arrow map

ffaafa1

clippy

1cf8888

Calculate map levels separately

d263051

Avoids the generic case of list > struct > [ley, value], which adds overhead

Fix map reader context and path

537c700

Map array tests

427ca9e

add doc comments and clean up code

52e17f9

wip: review feedback

0e2e424

add test for map

ef6944e

nevi-me force-pushed the map-support branch from 20ddfde to ef6944e Compare July 29, 2021 20:36

fix clippy 1.54 lints

d4d2985

alamb reviewed Jul 30, 2021

View reviewed changes

nevi-me merged commit 9be938e into apache:master Jul 31, 2021

nevi-me deleted the map-support branch August 1, 2021 07:20

nevi-me mentioned this pull request Aug 3, 2021

Support arrow map datatype #395

Closed

4 tasks

alamb mentioned this pull request Sep 9, 2021

Update DataFusion to arrow 6.0 apache/datafusion#984

Merged

tustvold mentioned this pull request Dec 7, 2021

Implement returning dictionary arrays from parquet reader #171

Closed

alamb mentioned this pull request Jan 11, 2022

Nicer API to create MapArrays #1158

Closed

wjones127 mentioned this pull request Jul 10, 2022

Support FFI / C Data Interface for MapType #2037

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal MapArray support #491

Minimal MapArray support #491

nevi-me commented Jun 22, 2021 •

edited

Loading

codecov-commenter commented Jul 3, 2021 •

edited

Loading

nevi-me commented Jul 11, 2021

alamb commented Jul 12, 2021

alamb left a comment

alamb Jul 13, 2021

nevi-me Jul 29, 2021

alamb Jul 13, 2021

nevi-me Jul 22, 2021

alamb Jul 13, 2021

nevi-me Jul 22, 2021

alamb Jul 13, 2021

nevi-me Jul 29, 2021

nevi-me commented Jul 14, 2021

nevi-me commented Jul 29, 2021

alamb left a comment

houqp commented Jul 31, 2021

nevi-me commented Sep 1, 2021

houqp commented Sep 7, 2021

alamb commented Sep 9, 2021

Minimal MapArray support #491

Minimal MapArray support #491

Conversation

nevi-me commented Jun 22, 2021 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Jul 3, 2021 • edited Loading

Codecov Report

nevi-me commented Jul 11, 2021

alamb commented Jul 12, 2021

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

nevi-me commented Jul 14, 2021

nevi-me commented Jul 29, 2021

alamb left a comment

Choose a reason for hiding this comment

houqp commented Jul 31, 2021

nevi-me commented Sep 1, 2021

houqp commented Sep 7, 2021

alamb commented Sep 9, 2021

nevi-me commented Jun 22, 2021 •

edited

Loading

codecov-commenter commented Jul 3, 2021 •

edited

Loading