GH-42218: [Java] MapVector cannot be loaded via IPC #43014

vibhatha · 2024-06-24T00:23:43Z

Rationale for this change

The MapVector keeps the KEY_NAME and VALUE_NAME as a constant values but rather it should be extracted from the provided fields.

What changes are included in this PR?

Updating KEY_NAME and VALUE_NAME from the provided fields. Adding a test case to validate that.

Add IPC Test Cases
Add C Data Test Cases

Are these changes tested?

Yes. A test case has been added to validate this.

Are there any user-facing changes?

No

GitHub Issue: [Java] MapVector cannot be loaded via IPC arrow-java#71
GitHub Issue: ARROW-190: Python: Provide installable sdist builds #71
GitHub Issue: ARROW-190: Python: Provide installable sdist builds #71

lidavidm

Let's not add mutable global state

lidavidm · 2024-06-25T00:34:53Z

Also, we should test this via IPC and C Data

vibhatha · 2024-06-25T01:05:40Z

Let's not add mutable global state

Would it be okay if we use getKey() across the static usages? That is going to be a larger diff.

java/vector/src/main/codegen/templates/UnionMapWriter.java

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

java/vector/src/main/codegen/templates/UnionMapWriter.java

lidavidm · 2024-06-27T01:22:19Z

java/vector/src/main/codegen/templates/UnionMapWriter.java

This should only be if there are 0 children? If there's any other number of children presumably it should fail

Right, so better to check size==0 and assign the KEY_NAME else, the field from the struct. Also for value, I think the most accurate check would be size < 2, because there could be no key defined, then there could be 1 child (when key was added).

java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java

lidavidm · 2024-06-27T01:25:38Z

java/c/src/test/java/org/apache/arrow/c/StreamTest.java

I'd like to see IPC and C Data integration tests with other languages, not within Java

@lidavidm this was an old issue filed for this purpose and we have a C Data interface test being skipped because of that. https://github.com/apache/arrow/issues/24869

I have enabled the non_canonical_map tests in archery integration tests. But I am getting one failure as shown below.

RuntimeError: Command failed: java -Dio.netty.tryReflectionSetAccessible=true -Darrow.struct.conflict.policy=CONFLICT_APPEND -XX:-UsePerfData --add-opens=java.base/java.nio=org.apache.arrow.memory.core,ALL-UNNAMED --add-reads=org.apache.arrow.flight.core=ALL-UNNAMED -cp /home/vibhatha/github/fork/arrow/java/tools/target/arrow-tools-17.0.0-SNAPSHOT-jar-with-dependencies.jar org.apache.arrow.tools.Integration -a /tmp/tmpj67jyv0g/8bbfe7eb_generated_map_non_canonical.consumer_stream_as_file -j /tmp/arrow-integration-7d6i1uz7/generated_map_non_canonical.json -c VALIDATE With output: -------------- WARNING: Unknown module: org.apache.arrow.flight.core specified to --add-reads WARNING: Unknown module: org.apache.arrow.memory.core specified to --add-opens SLF4J(W): No SLF4J providers were found. SLF4J(W): Defaulting to no-operation (NOP) logger implementation SLF4J(W): See https://www.slf4j.org/codes.html#noProviders for further details. Incompatible files Different schemas: Schema<map_other_names: Map(false)<entries: Struct<key: Utf8 not null, value: Int(32, true)> not null>> Schema<map_other_names: Map(false)<some_entries: Struct<some_key: Utf8 not null, some_value: Int(32, true)> not null>>

I checked two things,

The schema of the Arrow file is Schema<map_other_names: Map(false)<entries: Struct<key: Utf8 not null, value: Int(32, true)> not null>>

Verified by just reading the schema of the loaded file.

I independently checked whether Java JsonReader/Writer can write custom key and value fields.

It can write and read custom fields, I think that's why the Java Producing and Java consuming passes.
Also Java Producing and C++ consuming tests are passing.

But in C++ producing and Java consuming, I get the above error. Also in 1 I checked the schema, and it is what is being noted
Schema<map_other_names: Map(false)<entries: Struct<key: Utf8 not null, value: Int(32, true)> not null>>.

I have a doubt as it seems C++ is not producing the custom schema? Am I missing something here?

# Canonical map names are restored on import, so the schemas are unequal .skip_format(SKIP_C_SCHEMA, 'C++') .skip_format(SKIP_C_SCHEMA, 'Java'),

there's a comment presumably referencing this. Maybe @pitrou can clarify?

I added the Java line, though still fails. @pitrou appreciate your feedback?

C++ only compares map field names if the check_metadata option is enabled (it's disabled by default). Java should probably do something similar.

arrow/cpp/src/arrow/compare.cc

Lines 791 to 796 in 8fc40fc

if (check_metadata_ && (left.item_field()->name() != right.item_field()->name() ||

left.key_field()->name() != right.key_field()->name() ||

left.value_field()->name() != right.value_field()->name())) {

result_ = false;

return Status::OK();

}

Thanks @pitrou for investigating this. I will take a look.

Looking into this, Java uses, the Integration.java program to do this validation as used by the archery. And in the program there is a schema comparison, just by using the content in the generated JSON files and arrow files. So this anyways should fail right? Because the JSON file generated has one schema, and the IPC file has another schema as the loaded data doesn't contain the canonical names in C++?

JSON Schema

"schema": { "fields": [ { "name": "map_other_names", "type": { "name": "map", "keysSorted": false }, "nullable": true, "children": [ { "name": "some_entries", "type": { "name": "struct" }, "nullable": false, "children": [ { "name": "some_key", "type": { "name": "utf8" }, "nullable": false, "children": [] }, { "name": "some_value", "type": { "name": "int", "isSigned": true, "bitWidth": 32 }, "nullable": true, "children": [] } ] } ] } ] },

IPC Content

>>> import pyarrow as pa >>> file_path = "/tmp/tmppa0h4f3m/7a269ab8_generated_map_non_canonical.consumer_stream_as_file" >>> with pa.OSFile(file_path, 'rb') as source: ... loaded_array = pa.ipc.open_file(source).read_all() ... >>> loaded_array pyarrow.Table map_other_names: map<string, int32> child 0, entries: struct<key: string not null, value: int32> not null child 0, key: string not null child 1, value: int32 ---- map_other_names: [[keys:["mw3gônj"]values:[null],null,keys:["矢gc6h43","4n矢3°5€"]values:[2147483647,-99106826],keys:["or3iµg£","naadofp","°dfhjrl","hôr£µn£"]values:[1403778175,401427101,null,1171070133],keys:["i2nk4oô"]values:[1754612069],keys:["wiôgid6","dÂom23r"]values:[1528772736,-696511786],keys:["rdÂ3n1r","wµrôÂfc","kfakmko","Â°2Â3hÂ"]values:[null,1836582554,-1502317905,153924375]]]

@vibhatha I'll repeat what I said above: C++ only compares map field names if the check_metadata option is enabled (it's disabled by default). Java should probably do something similar.

Note that Integration.java calls Validator.compareSchemas...

github-actions · 2024-11-26T19:13:45Z

⚠️ GitHub issue apache/arrow-java#71 has no components, please add labels for components.

kou · 2024-11-27T00:52:40Z

Could you open a new PR in apache/arrow-java?

vibhatha · 2024-11-27T01:18:14Z

Sure I will look into it.

github-actions · 2024-11-27T10:19:08Z

⚠️ GitHub issue #42218 has no components, please add labels for components.

github-actions bot added Component: Java awaiting review Awaiting review labels Jun 24, 2024

vibhatha marked this pull request as ready for review June 24, 2024 01:15

vibhatha requested a review from lidavidm as a code owner June 24, 2024 01:15

lidavidm requested changes Jun 25, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 25, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 25, 2024

vibhatha commented Jun 25, 2024

View reviewed changes

java/vector/src/main/codegen/templates/UnionMapWriter.java Outdated Show resolved Hide resolved

vibhatha requested a review from lidavidm June 25, 2024 11:03

lidavidm requested changes Jun 27, 2024

View reviewed changes

vibhatha force-pushed the gh-42218 branch from 91b9e2c to 8fce14b Compare June 28, 2024 00:18

vibhatha requested review from assignUser, kou and raulcd as code owners June 28, 2024 00:18

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 28, 2024

vibhatha added 7 commits July 23, 2024 14:19

fix: initial commit

5d24b02

fix: minor

b2d6765

fix: minor change to update key and value names

27e770a

fix: addressing reviews v1

e5be0a4

fix: adding c data interface roundtrip

0a936df

fix: adding c data interface test

37a7341

fix: addressing reviews v3

20ed1fc

vibhatha force-pushed the gh-42218 branch from 8fce14b to 20ed1fc Compare July 23, 2024 08:49

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 23, 2024

vibhatha requested a review from pitrou August 8, 2024 02:12

kou closed this Nov 27, 2024

	if (check_metadata_ && (left.item_field()->name() != right.item_field()->name() \|\|
	left.key_field()->name() != right.key_field()->name() \|\|
	left.value_field()->name() != right.value_field()->name())) {
	result_ = false;
	return Status::OK();
	}

GH-42218: [Java] MapVector cannot be loaded via IPC #43014

GH-42218: [Java] MapVector cannot be loaded via IPC #43014

Uh oh!

Conversation

vibhatha commented Jun 24, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Jun 25, 2024

Uh oh!

vibhatha commented Jun 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

github-actions bot commented Nov 26, 2024

Uh oh!

kou commented Nov 27, 2024

Uh oh!

vibhatha commented Nov 27, 2024

Uh oh!

github-actions bot commented Nov 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vibhatha commented Jun 24, 2024 •

edited by github-actions bot

Loading