ARROW-6111: [Java] Support LargeVarChar and LargeBinary types #6425

liyafan82 · 2020-02-14T13:06:51Z

These types of vectors are also variable width vectors. However, they have a offset width of 8, so the underlying data buffer can be over 2GB in size.

github-actions · 2020-02-14T13:16:42Z

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

emkornfield · 2020-02-24T06:03:32Z

@siddharthteotia do you have time to review?

emkornfield · 2020-03-06T04:59:48Z

@BryanCutler if you have time to review it would be appreciated otherwise I can take a look soon.

siddharthteotia · 2020-03-06T05:31:28Z

I will review this tomorrow. Sorry for the delay.

emkornfield · 2020-03-11T05:38:46Z

@siddharthteotia do you still intend to review?

siddharthteotia · 2020-03-12T05:41:35Z

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

shouldn't this be long?

My bad -- It's the offset that has to be long, not this index since this is for the number of values.

@siddharthteotia Thanks a lot for your attention. Currently, we are using int32 for all vector indices, and int64 for all ArrowBuf indices.

siddharthteotia · 2020-03-12T05:44:23Z

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

Does it make sense for this to extend BaseVariableWidthVector?

We might be able to reuse quite some functionality. However, generally in Arrow we have been okay with some code duplication to avoid the performance overhead of having a deep inheritance hierarchy. So may be the current approach is fine. Just wondering if we evaluated this.

Good question.
We choose to have a separate BaseLargeVariableWidthVector class after careful thoughts and evaluations.

It appears that BaseVariableWidthVector and BaseLargeVariableWidthVector have many similarities. However, they are different in details, and the differences cannot be overcome by method overriding/overloading.

For example, in BaseVariableWidthVector, we have

protected final int getStartOffset(int index)

while in BaseLargeVariableWidthVector, we have

protected final long getStartOffset(int index)

These two methods differ only in return type, so we cannot use method overloading/overriding.

Another concern is performance. For example, handleSafe is a performance critical operation, because it may be called in each setSafe method call. So it is declared as a final method. If we share the same base class (BaseVariableWidthVector), we must provide two implementations for handleSafe, and it will be virtual instead of being final. This may lead to performance degradation.

nealrichardson · 2020-04-09T15:29:39Z

@liyafan82 what's the status of this? Are you intending to get this into 0.17?

liyafan82 · 2020-04-10T04:16:46Z

@liyafan82 what's the status of this? Are you intending to get this into 0.17?

@nealrichardson Thanks for your attention.
This PR is being reviewed. I am not sure if the reviewing process can finish in time.

liyafan82 · 2020-04-10T04:28:39Z

@siddharthteotia Do you have any more comments to this PR?

BryanCutler

Made a quick pass and mostly looks good, I'll try to look at more detail soon.

BryanCutler · 2020-04-15T21:15:21Z

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

couldn't you combine this with the "VarChar" block?

Revised. Thanks for the good suggestion.

BryanCutler · 2020-04-15T21:15:27Z

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

Revised. Thank you.

BryanCutler · 2020-04-16T02:18:09Z

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

Is this meant to be implemented later? maybe add some comment and a message

Sounds good. Added some comments here.

BryanCutler · 2020-04-16T02:19:14Z

java/vector/src/main/java/org/apache/arrow/vector/util/VectorAppender.java

Comments added. Thank you.

BryanCutler · 2020-04-16T02:20:57Z

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

So do we need #6323 to properly test out the 64bit offset?

Sure. I will add integration test later, when this or that PR finishes.

emkornfield · 2020-04-29T05:45:00Z

@BryanCutler @liyafan82 can this be merged now?

liyafan82 · 2020-04-29T07:52:15Z

@BryanCutler @liyafan82 can this be merged now?

@emkornfield Since ARROW-7610 is finished, we need to add an integration test for LargeVarChar/LargeBinary in this PR (or in another PR)?

liyafan82 · 2020-04-30T13:14:01Z

I have added integration test for the large varchar vector.

nealrichardson · 2020-05-01T16:27:24Z

If you're doing integration tests as part of this patch, please remove this skip: https://github.com/apache/arrow/blob/master/dev/archery/archery/integration/datagen.py#L1446

BryanCutler · 2020-05-01T20:31:20Z

Yes, please enable integration tests and lets make sure it passes before merging this.

liyafan82 · 2020-05-06T03:45:36Z

@nealrichardson and @BryanCutler Thanks for your good suggestion.
I have removed the skip in datagen.py, but the tests failed. The reason is that we do not support LargeList type yet:

2020-05-06T03:03:50.3032543Z com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Could not resolve type id 'largelist' as a subtype of [simple type, class org.apache.arrow.vector.types.pojo.ArrowType]: known type ids = [binary, bool, date, decimal, duration, fixedsizebinary, fixedsizelist, floatingpoint, int, interval, largebinary, largeutf8, list, map, null, struct, time, timestamp, union, utf8] (for POJO property 'type')
2020-05-06T03:03:50.3033308Z  at [Source: (File); line: 7, column: 19] (through reference chain: org.apache.arrow.vector.types.pojo.Schema["fields"]->java.util.ArrayList[0]->org.apache.arrow.vector.types.pojo.Field["type"])

I have revised the comment in datagen.py accordingly.

emkornfield · 2020-05-15T04:19:55Z

@BryanCutler @siddharthteotia I think I'm OK merging if this is you are happy with the code and following up on integation tests as part of ARROW-6110? What do you two think?

BryanCutler · 2020-05-15T18:46:08Z

I think you should be removing the skip Java here https://github.com/apache/arrow/blob/2f72713446b04f8979b04f907e7185985028b0a8/dev/archery/archery/integration/datagen.py#L1480 to enable integration testing for this. I'll try to take another review pass early next week.

emkornfield · 2020-05-15T18:49:58Z

@BryanCutler I believe the integration tests couple LargeList with LargeVarChar/LargeBinary, so both need to be implemented to enable the integration tests.

BryanCutler · 2020-05-18T21:27:01Z

The generate_primitive_large_offsets_case looks like it is just testing 'largebinary', 'largeutf8'. Are large lists somehow part of that?

def generate_primitive_large_offsets_case(batch_sizes):
    types = ['largebinary', 'largeutf8']

    fields = []

    for type_ in types:
        fields.append(get_field(type_ + "_nullable", type_, nullable=True))
        fields.append(get_field(type_ + "_nonnullable", type_, nullable=False))

    return _generate_file('primitive_large_offsets', fields, batch_sizes

emkornfield · 2020-05-18T21:35:56Z

@BryanCutler you are correct, for some reason I thought LargeList was coupled with LargeVarChar/LargeBinary.

liyafan82 · 2020-05-22T09:49:32Z

@BryanCutler @emkornfield Sorry for my late response.
I have removed the skip. Let's see if the integration tests can pass this time.

BryanCutler · 2020-05-22T19:54:26Z

Thanks @liyafan82 , looks like they didn't pass on this first try. Any idea what was causing the error?

Error accessing files
Current token (VALUE_STRING) not numeric, can not use numeric value accessors
 at [Source: (File); line: 65, column: 14]
12:46:37.776 [main] ERROR org.apache.arrow.tools.Integration - Error accessing files
com.fasterxml.jackson.core.JsonParseException: Current token (VALUE_STRING) not numeric, can not use numeric value accessors
 at [Source: (File); line: 65, column: 14]
	at com.fasterxml.jackson.core.JsonParser._constructError(JsonParser.java:1804)
	at com.fasterxml.jackson.core.base.ParserMinimalBase._reportError(ParserMinimalBase.java:698)
	at com.fasterxml.jackson.core.base.ParserBase._parseNumericValue(ParserBase.java:781)
	at com.fasterxml.jackson.core.base.ParserBase._parseIntValue(ParserBase.java:799)
	at com.fasterxml.jackson.core.base.ParserBase.getIntValue(ParserBase.java:645)
	at org.apache.arrow.vector.ipc.JsonFileReader$BufferHelper$5.read(JsonFileReader.java:306)

liyafan82 · 2020-05-23T04:14:06Z

Thanks @liyafan82 , looks like they didn't pass on this first try. Any idea what was causing the error?

Error accessing files
Current token (VALUE_STRING) not numeric, can not use numeric value accessors
 at [Source: (File); line: 65, column: 14]
12:46:37.776 [main] ERROR org.apache.arrow.tools.Integration - Error accessing files
com.fasterxml.jackson.core.JsonParseException: Current token (VALUE_STRING) not numeric, can not use numeric value accessors
 at [Source: (File); line: 65, column: 14]
	at com.fasterxml.jackson.core.JsonParser._constructError(JsonParser.java:1804)
	at com.fasterxml.jackson.core.base.ParserMinimalBase._reportError(ParserMinimalBase.java:698)
	at com.fasterxml.jackson.core.base.ParserBase._parseNumericValue(ParserBase.java:781)
	at com.fasterxml.jackson.core.base.ParserBase._parseIntValue(ParserBase.java:799)
	at com.fasterxml.jackson.core.base.ParserBase.getIntValue(ParserBase.java:645)
	at org.apache.arrow.vector.ipc.JsonFileReader$BufferHelper$5.read(JsonFileReader.java:306)

Thank you @BryanCutler . I am investigating.

liyafan82 · 2020-05-26T08:25:35Z

@BryanCutler The integration tests pass now. Please take another look when you have time. Thank you.

BryanCutler

Great job getting integration tests passing @liyafan82 ! I took a quick look and LGTM, will do a more thorough pass hopefully tomorrow before merging.

liyafan82 · 2020-05-27T06:03:00Z

Great job getting integration tests passing @liyafan82 ! I took a quick look and LGTM, will do a more thorough pass hopefully tomorrow before merging.

@BryanCutler Please take your time. Thanks a lot for your effort.

siddharthteotia

Just did a final pass. LGTM. Thanks @BryanCutler for the review.
Thanks @liyafan82 for seeing this through. Good job

BryanCutler

LGTM, thanks for taking a look also @siddharthteotia

BryanCutler · 2020-05-27T21:46:48Z

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


+  @Override
+  public Void visit(BaseLargeVariableWidthVector left, Void value) {
+    return null;


is this another TODO? Would you mind making JIRAs for this and the other TODOs here so they can be tracked?

Sure. I will fix this problem in ARROW-8402, and will open other JIRAs to track other TODOs. Thanks for your reminder.

BryanCutler · 2020-05-27T21:54:43Z

merged to master, thanks @liyafan82 !

liyafan82 · 2020-05-28T02:11:07Z

@BryanCutler @siddharthteotia @emkornfield I am aware that this PR is large, and consumes lots of effort. So thanks a lot for your effort and good comments!

These types of vectors are also variable width vectors. However, they have a offset width of 8, so the underlying data buffer can be over 2GB in size. Closes apache#6425 from liyafan82/fly_0210_large Authored-by: liyafan82 <fan_li_ya@foxmail.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>

siddharthteotia reviewed Mar 12, 2020

View reviewed changes

fsaintjacques added the Component: Java label Mar 23, 2020

liyafan82 force-pushed the fly_0210_large branch from 941281d to 820b0a9 Compare April 10, 2020 04:27

BryanCutler reviewed Apr 16, 2020

View reviewed changes

wesm force-pushed the master branch from 5fe5b88 to aa55967 Compare April 19, 2020 22:47

kszucs force-pushed the master branch from 1b71ca7 to 5093b80 Compare April 20, 2020 19:21

liyafan82 force-pushed the fly_0210_large branch from 855eae1 to 819c306 Compare April 30, 2020 13:11

liyafan82 force-pushed the fly_0210_large branch 2 times, most recently from abf9ac4 to 2f72713 Compare May 6, 2020 03:43

[ARROW-6111][Java] Support LargeVarChar and LargeBinary types

c08cb66

liyafan82 added 3 commits May 22, 2020 15:59

[ARROW-6111][Java] Apply changes for the fix of ARROW-7837

b5ff33a

[ARROW-6111][Java] Add comments for future todo work

40c6a59

[ARROW-6111][Java] Add integration for large varchar vector

6d9f67a

liyafan82 force-pushed the fly_0210_large branch 2 times, most recently from ada09de to f795c56 Compare May 22, 2020 09:47

[ARROW-6111][Java] Revise integration tests

6e12363

liyafan82 force-pushed the fly_0210_large branch from f795c56 to 6e12363 Compare May 22, 2020 12:31

liyafan82 force-pushed the fly_0210_large branch 3 times, most recently from 25c403f to f429bd1 Compare May 26, 2020 07:08

liyafan82 closed this May 26, 2020

liyafan82 reopened this May 26, 2020

liyafan82 force-pushed the fly_0210_large branch from f429bd1 to 8e52912 Compare May 26, 2020 07:54

[ARROW-6111][Java] Fix integration test problems

b2bffe0

liyafan82 force-pushed the fly_0210_large branch from 8e52912 to b2bffe0 Compare May 27, 2020 02:19

BryanCutler reviewed May 27, 2020

View reviewed changes

siddharthteotia approved these changes May 27, 2020

View reviewed changes

BryanCutler approved these changes May 27, 2020

View reviewed changes

BryanCutler closed this in 51108e3 May 27, 2020

asfimport mentioned this pull request Jul 29, 2020

[Java] Support LargeVarChar and LargeBinary types and add integration test with C++ #22508

Closed

ARROW-6111: [Java] Support LargeVarChar and LargeBinary types #6425

ARROW-6111: [Java] Support LargeVarChar and LargeBinary types #6425

Uh oh!

Conversation

liyafan82 commented Feb 14, 2020

Uh oh!

github-actions bot commented Feb 14, 2020

Uh oh!

emkornfield commented Feb 24, 2020

Uh oh!

emkornfield commented Mar 6, 2020

Uh oh!

siddharthteotia commented Mar 6, 2020

Uh oh!

emkornfield commented Mar 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyafan82 Mar 12, 2020 • edited by emkornfield 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!

liyafan82 Mar 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!

nealrichardson commented Apr 9, 2020

Uh oh!

liyafan82 commented Apr 10, 2020

Uh oh!

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

Uh oh!

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!

emkornfield commented Apr 29, 2020

Uh oh!

liyafan82 commented Apr 29, 2020

Uh oh!

liyafan82 commented Apr 30, 2020

Uh oh!

nealrichardson commented May 1, 2020

Uh oh!

BryanCutler commented May 1, 2020

Uh oh!

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

Uh oh!

Uh oh!

emkornfield commented May 15, 2020

Uh oh!

BryanCutler commented May 15, 2020

Uh oh!

emkornfield commented May 15, 2020

liyafan82 Mar 12, 2020 •

edited by emkornfield

Loading

liyafan82 Mar 12, 2020 •

edited

Loading

liyafan82 commented Apr 10, 2020 •

edited

Loading

liyafan82 commented May 6, 2020 •

edited

Loading

BryanCutler commented May 22, 2020 •

edited

Loading