ARROW-1279: [Integration] Enable MapType integration tests #4684

BryanCutler · 2019-06-25T01:26:02Z

This PR enables MapType integration tests for C++ and Java, and fixes remaining issues to pass. Previously, C++ created a MapType with a StructType field that is nullable, which is required to be non-nullable from the spec. The struct field has the name "entries" and the struct data pairs are named "key" and "value."

BryanCutler · 2019-06-25T01:28:13Z

cpp/src/arrow/type.cc

This is the name that Java has hardcoded as a final variable in the ListVector. It can be changed though, but both sides have to agree or the schema equality test fails if it is different.

How about "entries"?

Yeah, that sounds much better to me

BryanCutler · 2019-06-25T01:28:58Z

cc @wesm @pitrou @bkietz

wesm · 2019-06-25T01:30:13Z

cpp/src/arrow/type.cc

How about "entries"?

wesm · 2019-06-25T01:30:24Z

cpp/src/arrow/type.cc

Can we call this "value" instead of "item"?

@wesm The reason for "item" is that "value" is already used in the scope of ListArray; it refers to what will now be called entries. So after the renaming above map_array.values() returns the entries and map_array.items() returns the values. If we override map_array.values() to do the thing everyone will expect, then it's no longer safe to treat ListArrays as MapArrays. Honestly what I'd prefer is to also rename ListArray::values to ListArray::entries or items so we can maintain sane naming

@BryanCutler

Added a JIRA to track this for later https://issues.apache.org/jira/browse/ARROW-5745

@bkietz so is the naming here ok to be merged for now and then we can finalize it in ARROW-5745 as a followup?

@BryanCutler I think so, thanks

BryanCutler · 2019-06-25T07:16:06Z

java/vector/src/main/java/org/apache/arrow/vector/complex/MapVector.java

cc @pravindra on the name change for MapVector struct field from "$data$" to "entries"

BryanCutler · 2019-06-25T18:21:39Z

Seems like arrow-ipc-json-test is failing in CI tests, but it passes for me locally. I'm not quite sure what part is failing either from reading the logs. Looks to be maybe a name mismatch. Any ideas @wesm ?

36/84 Test  #41: arrow-ipc-json-test .......................***Failed    0.59 sec
Running arrow-ipc-json-test, redirecting output into /home/travis/build/apache/arrow/cpp-build/build/test-logs/arrow-ipc-json-test.txt (attempt 1/1)
Running main() from /home/travis/build/apache/arrow/cpp-build/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc
[==========] Running 22 tests from 4 test cases.
[----------] Global test environment set-up.
[----------] 1 test from TestJsonSchemaWriter
[ RUN      ] TestJsonSchemaWriter.FlatTypes
[       OK ] TestJsonSchemaWriter.FlatTypes (7 ms)
[----------] 1 test from TestJsonSchemaWriter (7 ms total)
[----------] 3 tests from TestJsonArrayWriter
[ RUN      ] TestJsonArrayWriter.PrimitiveTypes
[       OK ] TestJsonArrayWriter.PrimitiveTypes (10 ms)
[ RUN      ] TestJsonArrayWriter.NestedTypes
WARNING: Logging before InitGoogleLogging() is written to STDERR
F0625 07:53:24.362190 28355 json-internal.cc:1415]  Check failed: (it->value.GetString()) == (child_field->name())
*** Check failure stack trace: ***
/home/travis/build/apache/arrow/cpp/build-support/run-test.sh: line 97: 28355 Aborted                 (core dumped) $TEST_EXECUTABLE "$@" 2>&1
     28356 Done                    | $ROOT/build-support/asan_symbolize.py
     28357 Done                    | ${CXXFILT:-c++filt}
     28358 Done                    | $ROOT/build-support/stacktrace_addr2line.pl $TEST_EXECUTABLE
     28359 Done                    | $pipe_cmd 2>&1
     28360 Done                    | tee $LOGFILE
~/build/apache/arrow/cpp-build/src/arrow/ipc

For me locally

      Start 30: arrow-ipc-json-simple-test
27/56 Test #30: arrow-ipc-json-simple-test .........   Passed    0.06 sec
      Start 31: arrow-ipc-json-test
28/56 Test #24: arrow-compute-aggregate-test .......   Passed    0.59 sec
      Start 32: arrow-json-integration-test
29/56 Test #31: arrow-ipc-json-test ................   Passed    0.08 sec
...
56/56 Test  #2: arrow-array-test ...................   Passed    3.30 sec

100% tests passed, 0 tests failed out of 56

pitrou · 2019-06-25T18:22:46Z

@BryanCutler Did you compile in debug mode? This may be a debug-only check.

BryanCutler · 2019-06-25T18:23:47Z

I built in release, let me try debug

BryanCutler · 2019-06-25T18:33:04Z

Yup that was it @pitrou , I can reproduce it now, thanks!

BryanCutler · 2019-06-26T19:37:51Z

I rebased, to hopefully get the MacOS tests passing

wesm

+1, pending CI

emkornfield · 2019-06-27T07:25:32Z

merging

This PR enables MapType integration tests for C++ and Java, and fixes remaining issues to pass. Previously, C++ created a MapType with a StructType field that is nullable, which is required to be non-nullable from the spec. The struct field has the name "entries" and the struct data pairs are named "key" and "value." Author: Bryan Cutler <cutlerb@gmail.com> Closes apache#4684 from BryanCutler/map-type-integration-tests-ARROW-1279 and squashes the following commits: 6259a45 <Bryan Cutler> rename MapType fields for internal test 3077780 <Bryan Cutler> rename MapType struct field to 'entries' 3185af9 <Bryan Cutler> Fix MapType to create a non-nullable struct field

BryanCutler commented Jun 25, 2019

View reviewed changes

wesm reviewed Jun 25, 2019

View reviewed changes

BryanCutler commented Jun 25, 2019

View reviewed changes

BryanCutler force-pushed the map-type-integration-tests-ARROW-1279 branch from d8e2342 to f1545d3 Compare June 26, 2019 19:03

wesm approved these changes Jun 26, 2019

View reviewed changes

wesm force-pushed the map-type-integration-tests-ARROW-1279 branch 2 times, most recently from 9f985a9 to b74435f Compare June 27, 2019 04:33

BryanCutler added 3 commits June 26, 2019 22:36

Fix MapType to create a non-nullable struct field

3185af9

rename MapType struct field to 'entries'

3077780

rename MapType fields for internal test

6259a45

BryanCutler force-pushed the map-type-integration-tests-ARROW-1279 branch from b74435f to 6259a45 Compare June 27, 2019 05:38

emkornfield closed this in 19b9ccc Jun 27, 2019

BryanCutler deleted the map-type-integration-tests-ARROW-1279 branch June 27, 2019 18:54

asfimport mentioned this pull request Jun 27, 2019

[Integration][Java] Integration tests for Map type #17215

Closed

ARROW-1279: [Integration] Enable MapType integration tests #4684

ARROW-1279: [Integration] Enable MapType integration tests #4684

Uh oh!

Conversation

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

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!

BryanCutler commented Jun 25, 2019

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!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Jun 25, 2019

Uh oh!

pitrou commented Jun 25, 2019

Uh oh!

BryanCutler commented Jun 25, 2019

Uh oh!

BryanCutler commented Jun 25, 2019

Uh oh!

BryanCutler commented Jun 26, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Jun 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BryanCutler commented Jun 25, 2019 •

edited

Loading