ARROW-6110: [Java][Integration] Support LargeList Type and add integration test with C++ #7275

rymurr · 2020-05-26T14:45:32Z

Add large list and ensure it works with Integration tests. As noted in the JIRA
ticket this is rather limited as the underlying vector doesn't support int64 addressing

The important downcasts to int32 have been noted for a follow up once vectors with
long addresses are supported

github-actions · 2020-05-26T14:46:38Z

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

emkornfield · 2020-06-02T06:31:36Z

@BryanCutler would you have time to review?

emkornfield · 2020-06-02T06:31:45Z

@rymurr looks like this needs a rebase

rymurr · 2020-06-02T08:49:24Z

@rymurr looks like this needs a rebase

Thanks for the reminder @emkornfield, done.

BryanCutler · 2020-06-09T22:55:35Z

I'm a little swamped right now, but I'll try to review sometime this week

liyafan82 · 2020-06-12T11:52:31Z

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

Here we need to cast the argument of getLong to long, to avoid integer overflow.

liyafan82 · 2020-06-12T11:52:54Z

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

Here we need to cast the first argument, too.

liyafan82 · 2020-06-12T11:57:40Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

I think we can replace the old implementation

void setBit(ArrowBuf validityBuffer, int index)

with this one, as now ArrowBuf is based on 64-bit index.

please update the link in setValidityBitToOne here https://github.com/apache/arrow/pull/7275/files#diff-bc5e2a2a1b6b348c39f57c91defb855bL79

liyafan82 · 2020-06-12T11:59:09Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

Here we need to cast getLong argument to long.

liyafan82 · 2020-06-12T12:49:57Z

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

We need to cast (numRecords + 1) to long. Otherwise, integer overflow may happen before promoting the result to long.

nealrichardson · 2020-06-24T15:19:35Z

Is this good to merge now? @BryanCutler are you still planning to review this? Would like to get this in 1.0.

BryanCutler · 2020-06-30T19:03:17Z

Is this good to merge now? @BryanCutler are you still planning to review this? Would like to get this in 1.0.

I'm taking a look now, I'd like to get it in for 1.0 too.

BryanCutler

Thanks for working on this @rymurr ! Apologies for taking so long to review.. It looks pretty good, but I saw what looked like inconsistencies in the LargeListVector APIs using ints vs longs to me, and otherwise only minor things to fix up.

BryanCutler · 2020-06-30T17:35:10Z

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

It would be nice not to add a new template and combine with the UnionListWriter if possible. That doesn't have to be done here though, it can be looked at later.

agreed, I fixed this...was just being lazy ;-)

BryanCutler · 2020-06-30T17:40:55Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

please update the link in setValidityBitToOne here https://github.com/apache/arrow/pull/7275/files#diff-bc5e2a2a1b6b348c39f57c91defb855bL79

BryanCutler · 2020-06-30T17:56:02Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

Since this is the relative bit index, it's not possible to return long. Can you change return value to an int?

BryanCutler · 2020-06-30T17:56:30Z

java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java

From the comment above, I think this should still be an int

BryanCutler · 2020-06-30T18:20:18Z

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

shouldn't this be long?

BryanCutler · 2020-06-30T22:37:00Z

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

return int

BryanCutler · 2020-06-30T22:51:52Z

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

Could you add a TODO to revisit once 64 bit vectors are supported?

Also indicate this in the javadoc here and probably for the class. It might even be a good idea to raise an error if the user tries to add too many elements, otherwise things might just start looking wrong.

BryanCutler · 2020-06-30T22:54:02Z

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

This should return long and not be casted to int

BryanCutler · 2020-06-30T22:54:09Z

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

BryanCutler · 2020-06-30T23:01:31Z

java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileReader.java

could you add a check for Types.MinorType.LARGELIST here are remove the changes below?

you also won't need typeBitWidth as an arg

thanks, done

rymurr · 2020-07-01T10:02:03Z

Thanks for working on this @rymurr ! Apologies for taking so long to review.. It looks pretty good, but I saw what looked like inconsistencies in the LargeListVector APIs using ints vs longs to me, and otherwise only minor things to fix up.

Thanks a lot for the thorough review. I have fixed up everything you mentioned. It appears some of the confusion was related to changes for 64-bit allocations that were recently merged and the rest was my ignorance!

I have pushed a change with all your recommended fixes.

BryanCutler

Thanks for the quick update @rymurr , it looks pretty good! Only a couple minor things. I see quite a few instances of offsetBuffer.getLong/setLong(i * OFFSET_WIDTH) that I believe need to be cast to long to avoid overflow. Could you take a quick pass and fix those up? I think we will be good to go after that.

BryanCutler · 2020-07-02T19:31:44Z

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

does this arg need to be cast to long?

BryanCutler · 2020-07-02T19:31:51Z

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

BryanCutler · 2020-07-02T19:32:17Z

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

cast to long?

BryanCutler · 2020-07-02T19:42:02Z

java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileReader.java

was there a reason for this change?

IDE autoformatting...reverted!

BryanCutler · 2020-07-02T19:43:26Z

java/vector/src/main/java/org/apache/arrow/vector/complex/impl/UnionLargeListReader.java

why is this needed? the vector only supports integer max_value number of elements right?

you are correct. Removed

BryanCutler · 2020-07-02T19:48:06Z

java/vector/src/main/java/org/apache/arrow/vector/validate/ValidateVectorVisitor.java

doesn't value need to be passed in here?

value is always of type Void. None of the other visitors pass value to children.

rymurr · 2020-07-03T09:16:13Z

Thanks for the quick update @rymurr , it looks pretty good! Only a couple minor things. I see quite a few instances of offsetBuffer.getLong/setLong(i * OFFSET_WIDTH) that I believe need to be cast to long to avoid overflow. Could you take a quick pass and fix those up? I think we will be good to go after that.

Thanks for another thorough review @BryanCutler ! I think that I addressed everything. I may have went over the top w/ the casting but it doesn't hurt :-)

Add large list and ensure it works with Integration tests. As noted in the JIRA ticket this is rather limited as the underlying vector doesn't support int64 addressing The important downcasts to int32 have been noted for a follow up once vectors with long addresses are supported

BryanCutler

LGTM

BryanCutler · 2020-07-06T05:42:20Z

merged to master, thanks @rymurr !

…ation test with C++ Add large list and ensure it works with Integration tests. As noted in the JIRA ticket this is rather limited as the underlying vector doesn't support int64 addressing The important downcasts to int32 have been noted for a follow up once vectors with long addresses are supported Closes apache#7275 from rymurr/ARROW-6110 Authored-by: Ryan Murray <rymurr@dremio.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

fsaintjacques added the Component: Java label May 26, 2020

rymurr force-pushed the ARROW-6110 branch from 0d8d36d to db5ad4b Compare June 2, 2020 08:49

liyafan82 reviewed Jun 12, 2020

View reviewed changes

rymurr force-pushed the ARROW-6110 branch 3 times, most recently from 4383a6d to 8ca2cb2 Compare June 22, 2020 15:11

BryanCutler requested changes Jun 30, 2020

View reviewed changes

rymurr force-pushed the ARROW-6110 branch from 8ca2cb2 to 8ced1f2 Compare July 1, 2020 10:01

BryanCutler reviewed Jul 2, 2020

View reviewed changes

rymurr force-pushed the ARROW-6110 branch from 8ced1f2 to a60f447 Compare July 3, 2020 09:38

BryanCutler approved these changes Jul 6, 2020

View reviewed changes

BryanCutler closed this in 3f9f566 Jul 6, 2020

rymurr deleted the ARROW-6110 branch July 6, 2020 08:32

asfimport mentioned this pull request Jul 6, 2020

[Java] Support LargeList Type and add integration test with C++ #22507

Closed

ARROW-6110: [Java][Integration] Support LargeList Type and add integration test with C++ #7275

ARROW-6110: [Java][Integration] Support LargeList Type and add integration test with C++ #7275

Uh oh!

Conversation

rymurr commented May 26, 2020

Uh oh!

github-actions bot commented May 26, 2020

Uh oh!

emkornfield commented Jun 2, 2020

Uh oh!

emkornfield commented Jun 2, 2020

Uh oh!

rymurr commented Jun 2, 2020

Uh oh!

BryanCutler commented Jun 9, 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!

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!

nealrichardson commented Jun 24, 2020

Uh oh!

BryanCutler commented Jun 30, 2020

Uh oh!

BryanCutler left a comment

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!

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