ARROW-1692: [Java] UnionArray round trip not working #7290

rymurr · 2020-05-27T13:12:11Z

This fixes the integration tests for Union Arrays.

Both Dense and Sparse unions now work:

validity buffer added to Sparse Union
both Union types now track logical types rather than MinorType enum ordinal

Dependent on #7289 for duplicate field names.

github-actions · 2020-05-27T13:16:36Z

https://issues.apache.org/jira/browse/ARROW-1692

lidavidm · 2020-06-11T12:58:50Z

It looks like the integration tests are still failing for union arrays between C++ and Java.

rymurr · 2020-06-11T13:26:42Z

It looks like the integration tests are still failing for union arrays between C++ and Java.

yeah, something w/ Flight, though the IPC works fine. Checking what is different in flight now...

liyafan82 · 2020-06-12T09:58:33Z

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

Could you please write some comments about the indices & values of the array?

liyafan82 · 2020-06-12T10:20:13Z

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

do we need to multiply by 2 here?

removed. Probably a copy/paste error

liyafan82 · 2020-06-12T10:21:03Z

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

there should be a space after the cast, or the style check would fail?

liyafan82 · 2020-06-12T10:25:07Z

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

nit: Maybe we can use

setValidityBit(ArrowBuf validityBuffer, int index, int value)

here to make the code more concise?

liyafan82 · 2020-06-12T10:28:27Z

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

Here we are still relying on the MinorType ordinal as the type ID?
If so, the sparse union vector is still not aligning with the specification, and that causes the integration test to fail?

This switch uses the typeIds array to extract the MinorType associated with the logical type. If it does not exist in the array (usually because a logical type wasn't previously assigned) it defaults to the id of the minor type. So Java always uses the MinorType as the type id and allows c++ (and others) to send arbitrary type mappings.

emkornfield · 2020-06-12T15:02:45Z

@jacques-n I think you had some concerns the last time sparse unions were made. Do these changes raise the same issues?

nealrichardson · 2020-06-22T15:26:45Z

Assuming CI is still passing, is this good to merge? What is left to do, or who needs to approve?

emkornfield · 2020-06-23T04:43:39Z

It would be great if @jacques-n could confirm he is OK with these changes. I believe on a previous PR the extra indirection was a concern.

jacques-n · 2020-06-23T21:08:41Z

I'm really struggling with these changes. I don't understand why there is a validity buffer at the union level as well as at the cell level. I'm not sure what it even means that a a union value is valid but the inner value is not. I think there was an inconsistent understanding on my part when that change was proposed in the format (versus what the Java implementation was already doing). @wesm why would we have validity at both the top level and the inner level? Or is the C++ impl only at the top level and not the inner level (which means that this java code is incorrect since the inner vectors have their own validity vectors.

jacques-n · 2020-06-23T21:10:09Z

Adding to my previous comments: if only at the top level, I'm not sure what the ramification of that would mean at the Java codebase. I think it would require a fairly massive refactoring.

wesm · 2020-06-23T21:23:51Z

@wesm why would we have validity at both the top level and the inner level?

Well, the way the specification is written

All nested types including union are composed from well-formed child arrays which may have null values.
Additionally, all array types, including all nested array types, have their own validity bitmap
In the case of union, a null at the top level would indicate that the type of the child is not known. This seems algebraically consistent to me.

We can decide to stipulate that union types never have non-valid values at the Union cell level, only at the child cell level. But then a union value cannot be "made null" by changing the validity bitmap of the Union. From a purely algebraic / design perspective it isn't great. From the get-go in the project I've been striving for algebraic consistency, e.g. enabling well-formed arrays to be composed to created composite types without alteration. Since Unions are one of the more seldom used part of the project we can decide to explicitly deviate from that, but we need to do it now if we're going to do that and not delay longer in reconciling the issue.

wesm · 2020-06-23T21:32:23Z

FTR I'm OK with dropping the top-level validity bitmap from Union, especially if it helps us move forward

jacques-n · 2020-06-24T00:28:29Z

We can decide to stipulate that union types never have non-valid values at the Union cell level, only at the child cell level. But then a union value cannot be "made null" by changing the validity bitmap of the Union.

I believe a union can still express this with a child type null type, no? I think that is how we either modeled it or planned to model it no the java side.

All nested types including union are composed from well-formed child arrays which may have null values.

I'm in agreement on this. Decomposing would be complex.

In the case of union, a null at the top level would indicate that the type of the child is not known. This seems algebraically consistent to me.

I think it's where the model breaks down because of the weird situation where you actually need to evaluate two validity buffers to determine whether something is valid: the parent and the child. And an inconsistency would be really weird. As such, I'm think it would be better to avoid the top-level validity buffer.

FTR I'm OK with dropping the top-level validity bitmap from Union, especially if it helps us move forward

That would be my preference. It seems to ultimately reduce the risk of inconsistency and doesn't seem to have any functional loss (given the use of null type to indicate a non-alternatively-typed value). I also think this works well in the most common case of union types, e.g. two files where one has fieldA with schemaA and another where you have fieldA with schemaB. Compositing those two doesn't require some kind of introspection and AND'ing of the individual children to build an additional validity buffer (or simply setting true for all and then having an inconsistency with the child array) and allows a fast set of the type vector for each independent chunk.

wesm · 2020-06-24T00:31:17Z

That would be my preference.

I'm OK with this. We would need to act quickly to try to pull this off for the release. I can start a DISCUSS thread and work up a patch with the proposed changes to the specification

rymurr · 2020-06-24T13:11:45Z

Thanks @wesm and @jacques-n for the review. I will leave this up until consensus is reached on the format change. Please let me know if I can help w/ the c++ patch, would be happy to contribute!

wesm · 2020-06-24T14:57:53Z

I sent an e-mail to dev@ -- let's discuss there

liyafan82 · 2020-06-30T02:54:19Z

In addition to the problem of top level validity buffer, I think there is another problem to discuss: Java is using the ordinal of the minor type as the type id, and this is not aligning with the C++ implementation/specification.

This PR works around the problem by supporting two modes. However, the C++ and Java implementations are not really aligning, IMO. Do we have a plan to mitigate this?

emkornfield · 2020-06-30T04:01:33Z

@liyafan82 I think that is a good point. If it supports both modes I think that is a reasonable compromise for now as long as @jacques-n is OK with it. But we can maybe discuss further if needed.

rymurr · 2020-06-30T08:11:38Z

I think this is now inline with the spec. The Union/DenseUnion types now uses logical type ids in Java. Which is the same as in c++. The difference in a java created Union is that the type ids are chosen from a known index (minor type). But the java implementation doesn't rely on minor type ordinals strict ordering like before. Or at least the intention was to be fully in line with spec while maintaining a convenient default choice for type id. Drawing any meaning from type buffers alone in client code would be an error in my opinion. Hope that clarifies my intention w/ this change.

rymurr · 2020-07-01T16:19:33Z

This has been modified to incorporate the changes to Unions as proposed on the mailing list

lidavidm · 2020-07-08T18:36:24Z

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

It seems NullVector throws UnsupportedOperationException in this case - should we be consistent with that?

Ahh, yes. I copied UnionVector (which returns typeBuffer) but I didn't really like it. Fixed on both

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

lidavidm · 2020-07-08T18:40:40Z

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

This is inconsistent with getNullCount; this vector has nullCount == 0 but can have isNull return true regardless. C++ seems to always return false when there is no validity buffer.

I am ok with changing this to always return false. However I think this means anyone using a Union vector would have to extract the child and check for null as an extra step. All other nested vectors have a top level validity buffer. I don't know what the ramifications of changing this are?

I set isNull to always be false and all the tests seem to pass (with minor modifications). Take a look and let me know what you think.

I think from the viewpoint of code generically manipulating vectors, this makes more sense - the entries of the union are never null, but they may reference something that is null. I agree it's less convenient when working with a union array explicitly though.

lidavidm

LGTM. @liyafan82 do you have other comments?

rymurr · 2020-07-10T11:54:31Z

Just rebased to include the memory related changes. Avoid any surprise broken master builds. @liyafan82 think its ready to go?

lidavidm · 2020-07-10T12:32:37Z

Looks like unions fail in integration again.

lidavidm · 2020-07-10T12:52:31Z

Seems like Java needs to be excluded from testing the 0.17.1 "golden" reference union file. Also, Java produces data that C++ can't read, but Java can read the C++ files; this fails the IPC tests with Java producing, and the Flight tests with both Java producing and consuming (since Flight tests upload and download).

rymurr · 2020-07-10T13:01:41Z

Seems like Java needs to be excluded from testing the 0.17.1 "golden" reference union file. Also, Java produces data that C++ can't read, but Java can read the C++ files; this fails the IPC tests with Java producing, and the Flight tests with both Java producing and consuming (since Flight tests upload and download).

Wonder if its the MetadataVersion change that went in for c++ today? Looking now.

rymurr · 2020-07-10T15:21:39Z

It is indeed the V4 patch that has been submitted on the c++ side and not on the Java side. I think this means we need to submit your patch first @lidavidm?

lidavidm · 2020-07-10T15:30:58Z

Hmm - Java doesn't check the metadata version and the error makes it sound like it's layout-related? Oh wait. So now C++'s view of the union layout depends on the metadata version. Ok, then let's merge the Java V5 metadata change first.

rymurr · 2020-07-10T16:03:31Z

dev/archery/archery/integration/runner.py

This is a minor hack to remove Java from 0.17.1 union gold tests. Not the most elegant way to do it but doing it correctly would be a non-trivial change to archery imho. Thoughts?

IMO I think this is OK for now.

This fixes the integration tests for Union Arrays. Both Dense and Sparse unions now work: * validity buffer added to Sparse Union * both Union types now track logical types rather than MinorType enum ordinal

wesm · 2020-07-11T20:34:48Z

Rebased since #7685 was merged

wesm · 2020-07-11T22:47:25Z

Tests pass so I'm merging. Hooray!

rymurr · 2020-07-12T09:01:02Z

Thanks for the rebate @wesm! Glad we have finished this one!

This fixes the integration tests for Union Arrays. Both Dense and Sparse unions now work: * validity buffer added to Sparse Union * both Union types now track logical types rather than MinorType enum ordinal Dependent on apache#7289 for duplicate field names. Closes apache#7290 from rymurr/ARROW-1692 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Wes McKinney <wesm@apache.org>

fsaintjacques added the Component: Java label May 27, 2020

rymurr force-pushed the ARROW-1692 branch from b83b859 to 640116d Compare June 10, 2020 18:08

rymurr marked this pull request as ready for review June 10, 2020 18:09

rymurr force-pushed the ARROW-1692 branch from 640116d to 2a75aab Compare June 11, 2020 07:20

liyafan82 reviewed Jun 12, 2020

View reviewed changes

rymurr force-pushed the ARROW-1692 branch 3 times, most recently from 14db89f to 0781a84 Compare June 22, 2020 15:11

rymurr force-pushed the ARROW-1692 branch from 0781a84 to 3e15a40 Compare July 1, 2020 16:19

rymurr force-pushed the ARROW-1692 branch 4 times, most recently from cc11a2e to 0c8f9e3 Compare July 7, 2020 12:00

lidavidm mentioned this pull request Jul 8, 2020

ARROW-9362: [Java] Support reading/writing V5 MetadataVersion #7685

Closed

lidavidm reviewed Jul 8, 2020

View reviewed changes

rymurr force-pushed the ARROW-1692 branch from 0c8f9e3 to e9d3691 Compare July 9, 2020 09:57

lidavidm approved these changes Jul 9, 2020

View reviewed changes

rymurr force-pushed the ARROW-1692 branch from e9d3691 to e9c8c6c Compare July 10, 2020 11:53

rymurr force-pushed the ARROW-1692 branch from e9c8c6c to 141f456 Compare July 10, 2020 16:02

rymurr commented Jul 10, 2020

View reviewed changes

rymurr force-pushed the ARROW-1692 branch from 141f456 to ec20274 Compare July 11, 2020 12:26

ARROW-1692: [Java] UnionArray round trip not working

719cb5f

This fixes the integration tests for Union Arrays. Both Dense and Sparse unions now work: * validity buffer added to Sparse Union * both Union types now track logical types rather than MinorType enum ordinal

wesm force-pushed the ARROW-1692 branch from ec20274 to 719cb5f Compare July 11, 2020 20:34

wesm closed this in d2ddaa6 Jul 11, 2020

lidavidm mentioned this pull request Jul 12, 2020

ARROW-9284: [Java] fix getMinorTypeForArrowType with dense unions #7599

Closed

rymurr deleted the ARROW-1692 branch July 13, 2020 14:16

asfimport mentioned this pull request Jul 12, 2020

[Python, Java] UnionArray round trip not working #17700

Closed

ARROW-1692: [Java] UnionArray round trip not working #7290

ARROW-1692: [Java] UnionArray round trip not working #7290

Uh oh!

Conversation

rymurr commented May 27, 2020

Uh oh!

github-actions bot commented May 27, 2020

Uh oh!

lidavidm commented Jun 11, 2020

Uh oh!

rymurr commented Jun 11, 2020

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!

liyafan82 Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

emkornfield commented Jun 12, 2020

Uh oh!

nealrichardson commented Jun 22, 2020

Uh oh!

emkornfield commented Jun 23, 2020

Uh oh!

jacques-n commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacques-n commented Jun 23, 2020

Uh oh!

wesm commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jun 23, 2020

Uh oh!

jacques-n commented Jun 24, 2020

Uh oh!

wesm commented Jun 24, 2020

Uh oh!

rymurr commented Jun 24, 2020

Uh oh!

wesm commented Jun 24, 2020

Uh oh!

liyafan82 commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emkornfield commented Jun 30, 2020

Uh oh!

rymurr commented Jun 30, 2020

Uh oh!

rymurr commented Jul 1, 2020

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!

lidavidm left a comment

liyafan82 Jun 12, 2020 •

edited

Loading

jacques-n commented Jun 23, 2020 •

edited

Loading

wesm commented Jun 23, 2020 •

edited

Loading

liyafan82 commented Jun 30, 2020 •

edited

Loading