ARROW-1207: [C++] Implement MapArray, MapBuilder, MapType classes, and IPC support by bkietz · Pull Request #4352 · apache/arrow

bkietz · 2019-05-20T15:49:18Z

Implements MapArray as a subclass of ListArray, where each value in the list is a key: item pair. (This naming is not the most natural, but value is taken.)

MapType::keys_sorted() is currently stored but unused- for example MapBuilder does not check inserted keys for correct ordering. MapType is printed as map<utf8, int32> and map<int32, float64, keys_sorted> for unsorted, sorted keys respectively.

Map arrays are created with ArrayFromJSON by providing for each pair an array of length 2 containing the key and the mapped item (example).

wesm · 2019-05-20T18:38:49Z

Will review this when I can. Should we try to inherit MapArray from ListArray? Seems like it might make things easier but I will take a closer look

pitrou · 2019-05-28T10:24:35Z

@bkietz You should be able to fix the Travis-CI failures by rebasing from master.

codecov-io · 2019-05-28T18:01:16Z

Codecov Report

Merging #4352 into master will increase coverage by 0.84%.
The diff coverage is 65.83%.

@@            Coverage Diff             @@
##           master    #4352      +/-   ##
==========================================
+ Coverage   88.41%   89.26%   +0.84%     
==========================================
  Files         784      639     -145     
  Lines      100266    89293   -10973     
  Branches     1251        0    -1251     
==========================================
- Hits        88651    79705    -8946     
+ Misses      11379     9588    -1791     
+ Partials      236        0     -236

Impacted Files	Coverage Δ
cpp/src/arrow/type_traits.h	`85.71% <ø> (ø)`	⬆️
cpp/src/arrow/visitor.h	`50% <ø> (ø)`	⬆️
cpp/src/arrow/visitor_inline.h	`88.11% <ø> (ø)`	⬆️
cpp/src/arrow/ipc/metadata-internal.cc	`88.17% <0%> (-1.89%)`	⬇️
cpp/src/arrow/util/concatenate.cc	`88.05% <0%> (-3.46%)`	⬇️
cpp/src/arrow/scalar.cc	`48.83% <0%> (-7.92%)`	⬇️
cpp/src/arrow/visitor.cc	`0% <0%> (ø)`	⬆️
cpp/src/arrow/compute/kernels/take.cc	`89.32% <0%> (-1.77%)`	⬇️
cpp/src/arrow/scalar.h	`85.71% <0%> (-2.53%)`	⬇️
cpp/src/arrow/compare.cc	`79.31% <0%> (-1.53%)`	⬇️
... and 164 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 2d7838e...ac878bb. Read the comment docs.

bkietz · 2019-05-28T20:57:48Z

@pitrou rebased and green

pitrou

Thanks @bkietz . A couple more comments below.

cpp/src/arrow/compare.cc

cpp/src/arrow/ipc/json-simple.cc

cpp/src/arrow/ipc/json-internal.cc

cpp/src/arrow/ipc/writer.cc

pitrou · 2019-05-29T15:48:02Z

cpp/src/arrow/type-test.cc

"keys_sorted=true" perhaps?

If you prefer

docs/source/format/Layout.rst

cpp/src/arrow/array-list-test.cc

no tests yet

pitrou · 2019-06-11T16:36:00Z

@wesm @bkietz I have a question (perhaps late) about the type layout. Why does the map have a struct child array instead of directly having two child arrays? This means, for example, that a map array can theoretically have null key-value pairs...

pitrou

LGTM now, but see question about the type layout definition.

pitrou · 2019-06-11T16:32:17Z

cpp/src/arrow/ipc/metadata-internal.cc

+      if (children.size() != 1) {
+        return Status::Invalid("Map must have exactly 1 child field");
+      }
+      if (  // FIXME(bkietz) temporarily disabled: this field is sometimes read nullable


Do you know where the issue lies?

bkietz · 2019-06-11T16:38:39Z

@pitrou This allows MapArray to be a specialization of ListArray, with corresponding reuse of code (comparison, for example)

bkietz · 2019-06-11T16:42:25Z

Null pairs are disallowed with DCHECKs, and neither MapBuilder nor ArrayFromJSON will produce a MapArray with a null pair.

pitrou · 2019-06-11T17:07:54Z

Thanks. I was just curious. Since the definition was already in the format spec, I won't object :-)

pitrou · 2019-06-11T17:09:15Z

This is a nice step forward. @bkietz is there a JIRA open for the integration issue? If not, can you open one?

bkietz · 2019-06-11T17:12:43Z

@BryanCutler merged

JIRA for integration tests: https://issues.apache.org/jira/browse/ARROW-1279

BryanCutler · 2019-06-11T17:42:04Z

Nice! The Java MapVector is merged also, so I think we can just enable the integration test and hope for green :)
Were you going to make a PR @bkietz ?

BryanCutler · 2019-06-11T22:04:26Z

Well, I got the integration tests running and there was only 1 error!
C++ producing, Java consuming

==========================================================
Testing file /tmp/tmplmqwtdnv/generated_map.json
==========================================================

14:14:18.822 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: Map data should be a non-nullable struct type

Looks like the struct field was written as nullable

bkietz force-pushed the 1207-Implement-Map-logical-type branch 2 times, most recently from 59bb5e0 to d62efcf Compare May 22, 2019 15:30

pitrou changed the title ~~ARROW-1207: C++ implement map logical type~~ ARROW-1207: [C++] implement map logical type May 28, 2019

wesm changed the title ~~ARROW-1207: [C++] implement map logical type~~ ARROW-1207: [C++] Implement MapArray, MapBuilder, MapType classes May 28, 2019

wesm changed the title ~~ARROW-1207: [C++] Implement MapArray, MapBuilder, MapType classes~~ ARROW-1207: [C++] Implement MapArray, MapBuilder, MapType classes, and IPC support May 28, 2019

bkietz force-pushed the 1207-Implement-Map-logical-type branch from 64fdad9 to ac878bb Compare May 28, 2019 16:56

pitrou requested changes May 29, 2019

View reviewed changes

bkietz force-pushed the 1207-Implement-Map-logical-type branch from ac878bb to 45895f3 Compare June 4, 2019 15:50

bkietz added 18 commits June 10, 2019 11:56

add MapType to Layout.rst

47d95ef

add MapType and test its ToString

01214fb

Add keysSorted field

e9b34d0

add map() type factory

5e727e5

add checked_pointer_cast for unique_ptr

7fbbe70

first pass at MapArray, MapBuilder, MapScalar

f89da94

no tests yet

adding some tests and filling out Map*

4c11db9

MapArray isa ListArray

8049c51

fix: obj_ is not a pointer

a5c88a1

set keys_, items_

eb6db03

de-inline MapBuilder constructor

31930ff

run clang-format

1047a6d

fix MapScalar typos

c936ebd

add tests using and validating MapBuilder

a3be934

remove redundant null check

62dade0

Add IPC tests for Map

9b455e7

ListType isa MapType

2aaab29

cleanup of code which assumes map has 2 children

a0de551

bkietz added 3 commits June 10, 2019 11:56

disable map IPC tests for Java

1b74aa1

more cleanup, disable JS ipc tests as well

41b3016

explicitly disable map in flight test

9fb8700

bkietz force-pushed the 1207-Implement-Map-logical-type branch from 80d69bc to 9fb8700 Compare June 11, 2019 13:09

pitrou reviewed Jun 11, 2019

View reviewed changes

pitrou approved these changes Jun 11, 2019

View reviewed changes

pitrou closed this in dede1e6 Jun 11, 2019

bkietz deleted the 1207-Implement-Map-logical-type branch February 25, 2021 16:39

asfimport mentioned this pull request Jun 11, 2019

[C++] Implement Map logical type #16979

Closed

Conversation

bkietz commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented May 20, 2019

Uh oh!

pitrou commented May 28, 2019

Uh oh!

codecov-io commented May 28, 2019

Codecov Report

Uh oh!

bkietz commented May 28, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou May 29, 2019

Choose a reason for hiding this comment

Uh oh!

bkietz May 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitrou commented Jun 11, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

bkietz commented Jun 11, 2019

Uh oh!

bkietz commented Jun 11, 2019

Uh oh!

pitrou commented Jun 11, 2019

Uh oh!

pitrou commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkietz commented Jun 11, 2019

Uh oh!

BryanCutler commented Jun 11, 2019

Uh oh!

BryanCutler commented Jun 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bkietz commented May 20, 2019 •

edited

Loading

pitrou commented Jun 11, 2019 •

edited

Loading