ARROW-1476: [JAVA] Implement Final ValueVector Updates #1316

siddharthteotia · 2017-11-14T01:25:11Z

cc @jacques-n , @BryanCutler , @icexelloss

bug fixes and clean up done as part of testing in Dremio -- this is the 3rd patch I was referring to

wesm · 2017-11-14T02:17:17Z

I just rebased this after ARROW-1717 was merged

wesm · 2017-11-14T03:58:28Z

Can we try to fix up the checkstyle warnings as part of this process as well? There seems to be a bunch of indentation problems etc.

wesm · 2017-11-14T03:58:51Z

We'll want to rebase the vector refactor branch after this is merged as some of the tests are failing due to reasons that have been fixed in master

siddharthteotia · 2017-11-14T04:53:47Z

@wesm, yes we need to rebase this branch before merging into master. I have addressed indentation and line length issues.

Close apache#1164 Close apache#1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

Implementation of all scalar types and complex types with corresponding legacy versions. Closes apache#1203

integration tests

icexelloss · 2017-11-14T14:55:15Z

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

What's the difference of this function and

public ArrowBuf[] getBuffers(boolean clear)

?

@siddharthteotia What's your thoughts on this?

Ping @siddharthteotia, can you explain on the difference between the two methods and how do we need both as public API?

icexelloss · 2017-11-14T14:59:38Z

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

icexelloss · 2017-11-14T15:00:46Z

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

Why do we need to check valueCount > 0?

If we haven't done setValueCount() on the source vector before invoking transfer(), then valueCount is essentially 0. Doing target.setValueCount(this.valueCount) will corrupt the target vector if this.valueCount is 0.

icexelloss · 2017-11-14T15:06:50Z

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

It seems naming conversion wise, all methods that handles allocates new buffers are called "xxxSafe".

Since we have

public void safeSafe(int index, ...)

Does

public void setValiditySafe(int index, boolean valid)

Makes more sense for this function?

Not quite sure. Since this has always been setIndexDefined(), I just left it as is

icexelloss · 2017-11-14T15:18:19Z

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

I am curious what's the use case of this API, i.e, when do we want to just change the length?

@siddharthteotia what's your thought on this?

Ping @siddharthteotia , can you explain why do we need this as a public API? When would a user want to just sent length for an element?

icexelloss · 2017-11-14T15:20:41Z

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

Don't know if it's github issue, is the license format correct?

Corrected. Not a github issue.

@siddharthteotia The license format still doesn't look correct.

icexelloss · 2017-11-14T15:21:22Z

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

Should this class be under test or o.a.a.tools?

Ideally under tools but for some weird reason, I wasn't able to resolve the dependency w.r.t arrow tools in Dremio -- I was definitely missing something but for saving time just put it here.

I prefer if we can move this out of vector public class. If we don't have time to do it here, can we track this somewhere so we don't forget to move it before release?

Ping @siddharthteotia. Looks like this class is not used anywhere. Do we need this?

We use this in Dremio

If it's only used for Dremio testing, does it make more sense to stay in Dremio codebase?

This is not something new. Vector classes have had the API generateTestData() from the beginning. As part of refactoring, we decided to move it out of the main API of vectors and provide a separate API for populating vectors with dummy data.

The old generateTestData() API in vectors is deprecated and the class description implies that any users of the former should now be using GenerateSampleData.

I see. It makes more sense now, thanks for the explanation. Let's keep it then.

icexelloss · 2017-11-14T15:24:21Z

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

Are there unit tests for this kind of stuff?

Yes, we have unit tests for splitAndTransfer of bit vector (validity buffer). Also we have unit tests for split and transfer of other vectors (nullable fixed, var width and complex) and we exercise all the conditions.

icexelloss · 2017-11-14T15:27:59Z

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

How about something like

BitVectorHelper.setValidityBit(validityBuffer, thisIndex, from.get(fromIndex));

?

I am also a bit confused about

set(thisIndex, ...)

vs

BitVectorHelper.setValidityBit(validityBuffer, thisIndex, ...)

What's the difference?

Actually, nvm. It seems all copyFrom follows the same pattern.

icexelloss · 2017-11-14T15:33:38Z

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

Do we have a unit test for this?

We have some copy() related unit tests but in any case, I am planning to write more unit tests in the next couple of weeks to get overall good coverage. I don't think so I can get to them now because of time constraints.

Note that behavior of copyFrom() hasn't changed.

icexelloss · 2017-11-14T15:37:17Z

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

I think "offset" is well defined in format, so this method can probably just be:

getOffset(int index)

or just:

offsetBuffer.getInt(index * OFFSET_WIDTH)

Probably it consist with holder.end = ...

startOffset implies the starting point of VARCHAR element in the data buffer.

icexelloss · 2017-11-14T15:42:21Z

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

Indentation seems incorrect.

icexelloss · 2017-11-14T15:44:25Z

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

Indentation seems incorrect.

icexelloss · 2017-11-14T15:45:39Z

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

Can this be lifted to base class?

Probably but I thought BaseRepeatedValueVector on itself is of no use -- at least doesn't represent a materialized field. Moreover, even if we move to base class, we still have to set reader and writer index for validity buffer in sub class since the validity buffer is a property of list vector and not BaseRepeatedValueVector.

icexelloss · 2017-11-14T15:47:13Z

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

Indentation

icexelloss · 2017-11-14T15:48:20Z

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

Why do we need this method? Can valueCapacity and validityCapacity mismatch?

validity and offset move together regardless of the buffers in underlying data vector of list vector so functions like setNotNull(), startNewValue(), setValueCount() which are essentially safe methods need to determine if the inner buffers of list vector need to be reallocated or not. This decision can't be made based on getValueCapacity() since that also accounts for the value capacity of data vector. I think by mistake it is public. that needs to be corrected.

I see. Yeah let's make this private.

icexelloss · 2017-11-14T15:52:04Z

@siddharthteotia This looks good. I left some comments.

One thing is that it seems this patch has quite bit of bug fixes but not many new tests. Should we add tests for those bug fixes?

siddharthteotia · 2017-11-15T00:44:24Z

There was one more problem. When JsonFileReader was refactored recently while removing the static interfaces, we did not set the writer index on the buffers we are allocating for the vector. As we are parsing JSON and writing to buffers, we should be setting writer index before calling loadFieldBuffers().

The static interfaces were doing so but we missed doing that in the recent change

BryanCutler · 2017-11-15T01:58:12Z

FYI, I updated Spark to use these changes and tests pass for the most part. There are still a couple things I need to work out, but overall it is looking good from this side!

icexelloss · 2017-11-15T02:30:29Z

Oops, I can take a look at JsonFileReader tomorrow. @siddharthteotia which tests are failing because of that?

siddharthteotia · 2017-11-15T03:13:18Z

Fixing checkstyle for warning is probably a wasted effort especially when the warnings are coming from code that hasn't been modified as well. I have filed a JIRA to address this separately. With so many warnings dumped by mvn package checkstyle:check, it is slightly difficult to identify what to fix and what not.

Essentially everything needs to be fixed -- indentation, order of imports etc throughout the code base. I have done it for most parts of newly written code.

I am fine with doing it but can we do it separately? I have filed a JIRA to track this
https://issues.apache.org/jira/browse/ARROW-1813

wesm · 2017-11-15T03:15:28Z

Yes, let's do the style fixes separately. I think it's long overdue

icexelloss · 2017-11-15T03:26:30Z

@siddharthteotia How about we fix at least indentation, header and white spaces, end of file new lines for new files?

Other checkstyle warning we can fix it later, but I am concerned with adding lots of new files with incorrect indentation and white spaces.

siddharthteotia · 2017-11-15T06:22:24Z

@icexelloss , I have fixed indentation, license header and white spaces in new files. Going through the code again. Travis CI build passes.

siddharthteotia · 2017-11-15T07:06:45Z

Build passed.

icexelloss · 2017-11-15T14:47:58Z

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

 import org.apache.arrow.vector.schema.ArrowRecordBatch;
 import org.apache.arrow.vector.schema.ArrowVectorType;

+import javax.annotation.Nullable;


icexelloss · 2017-11-15T14:52:07Z

java/vector/src/main/java/org/apache/arrow/vector/file/json/JsonFileReader.java

+          buf.writeByte(parser.getByteValue());
+        }
+
+        buf.writerIndex(size);


Do we need this? buf.writeByte seems to update writerIndex already.

Yes we don't need.

icexelloss · 2017-11-15T15:02:48Z

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

-      setNotNull(index);
-      lastSet = index + 1;
-      return offsets.getAccessor().get(lastSet);
-    }


@siddharthteotia I think we want to make this private, is that correct?

icexelloss · 2017-11-15T15:06:47Z

Awesome.

@siddharthteotia I have re commented on all my previous comments that are not addressed. Can you take a look? Thanks!

icexelloss · 2017-11-15T15:25:50Z

@siddharthteotia There are still some indentation and white space warning on the some new vector files

org/apache/arrow/vector/BaseNullableFixedWidthVector.java
org/apache/arrow/vector/BaseNullableVariableWidthVector.java

you can see this by running:

mvn package checkstyle:checkstyle

and check the html file under

vector/target/site/checkstyle.html

Can you double check those please? Thanks. Otherwise style looks correct.

wesm · 2017-11-15T15:38:48Z

Looks like we are on the home stretch. I can merge later today when these last small items are addressed

icexelloss · 2017-11-15T19:23:39Z

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

+   *         vector instance.
+   */
+  @Override
+  public ArrowBuf[] getBuffers(boolean clear) {


My previous comments are squashed because of code change.

Why do we need both

public ArrowBuf[] getBuffers(boolean clear)

and

public List<ArrowBuf> getFieldBuffers()

Both as public API? They look very similar to me and might confuse users.

AFAIK, getBuffers() is an old method for getting a batch of buffers for serialization. We use it for maintaining compatibility with Drill client. I think in future, we should consider removing it from Arrow and writing a wrapper over it in Dremio. This way we will just have getFieldBuffers.

I see. Does it make sense to mark getBuffers() as deprecated so it's less confusing for the other users?

Let's not worry about it right now. These two APIs have not been introduced as part of refactoring and have been in Arrow code for quite some time.

I will file a JIRA and we can brainstorm w.r.t appropriate steps to address the ambiguity (if any)

Ok. Sounds good.

icexelloss · 2017-11-15T19:28:18Z

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

+   * @param index   position of the element to set
+   * @param length  length of the element
+   */
+  public void setValueLengthSafe(int index, int length) {


This function is not used.

Also, I am not sure if we need an API for just setting value length. When would this be useful?

@siddharthteotia what's your thought on this

We use it in our Parquet code -- I may have to evaluate its usage but the API has been there in variable width vector from the beginning.

Thanks for the clarification - I wasn't sure which APIs are from the old vectors and which APIs are added.

Looks good then.

siddharthteotia · 2017-11-15T20:00:00Z

@wesm , let me know if I need to squash the latest 7 commits for this JIRA or you can take care of it while merging. I think we would be merging 4 commits into master:

ARROW-1473
ARROW-1474
ARROW-1717
ARROW-1476 (squashed latest commits)

siddharthteotia · 2017-11-15T20:31:41Z

Build passed.
I think we are all set for merge.

icexelloss · 2017-11-15T20:55:52Z

LGTM. +1.

Thanks @siddharthteotia for the epic effort!

wesm · 2017-11-15T21:21:27Z

Sweet, thanks all! I will take care of merging to master (and squashing the commits between the last refactor patch and now)

BryanCutler · 2017-11-15T21:26:55Z

+1, great work @siddharthteotia !

wesm · 2017-11-15T21:44:04Z

merged to master. thank you!

…s zero null count and optional validity buffer Currently when a Field has null count = 0, C++ will omit the validity buffer as it is optional in this case. Testing for #1316 was failing because Java was not handling this properly. This PR adds an explicit test to ensure this is being tested and easier to locate possible issues. Author: Bryan Cutler <cutlerb@gmail.com> Closes #1323 from BryanCutler/integration-test-optional-validity-buffer-ARROW-1821 and squashes the following commits: 4295660 [Bryan Cutler] expanded simple.json static test file to include field with null count = 0

wesm force-pushed the ARROW-1476 branch from a370421 to 980f1ed Compare November 14, 2017 02:17

siddharthteotia force-pushed the ARROW-1476 branch 2 times, most recently from 68066db to 9b18879 Compare November 14, 2017 04:51

siddharthteotia and others added 6 commits November 13, 2017 21:09

ARROW-1473: ValueVector new hierarchy prototype (implementation phase 1)

6493ebf

Close apache#1164 Close apache#1198 Change-Id: If18e42d2edfdfef83e83621334a5b65a390e9db9

ARROW-1474:[JAVA] ValueVector hierarchy (Implementation Phase 2)

4509c29

Implementation of all scalar types and complex types with corresponding legacy versions. Closes apache#1203

ARROW-1717: [Java] Refactor JsonReader for new class hierarchy and fix

0de6e69

integration tests

ARROW-1476: [JAVA] Implement Final ValueVector Updates

2027f0b

indentation and line length

102ff97

rebase on master, fix tests

343f47b

icexelloss reviewed Nov 14, 2017

View reviewed changes

integration test fix, some checkstyle fixes

9706172

siddharthteotia force-pushed the ARROW-1476 branch from af14dd7 to 9706172 Compare November 15, 2017 03:21

icexelloss reviewed Nov 15, 2017

View reviewed changes

icexelloss mentioned this pull request Nov 15, 2017

Add checkstyle file and license template siddharthteotia/arrow#2

Closed

icexelloss reviewed Nov 15, 2017

View reviewed changes

review comments

44b9dc1

icexelloss reviewed Nov 15, 2017

View reviewed changes

indentation and javadocs

77c3ba3

icexelloss reviewed Nov 15, 2017

View reviewed changes

wesm closed this Nov 15, 2017

BryanCutler mentioned this pull request Nov 16, 2017

ARROW-1821: [INTEGRATION] Add integration test case for when Field has zero null count and optional validity buffer #1323

Closed

ARROW-1476: [JAVA] Implement Final ValueVector Updates #1316

ARROW-1476: [JAVA] Implement Final ValueVector Updates #1316

Uh oh!

Conversation

siddharthteotia commented Nov 14, 2017

Uh oh!

wesm commented Nov 14, 2017

Uh oh!

wesm commented Nov 14, 2017

Uh oh!

wesm commented Nov 14, 2017

Uh oh!

siddharthteotia commented Nov 14, 2017 • 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!

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!

icexelloss Nov 14, 2017 • 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!

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!

icexelloss Nov 14, 2017 • 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!

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!

icexelloss Nov 14, 2017 • 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

siddharthteotia commented Nov 14, 2017 •

edited

Loading

icexelloss Nov 14, 2017 •

edited

Loading

icexelloss Nov 14, 2017 •

edited

Loading

icexelloss Nov 14, 2017 •

edited

Loading

icexelloss Nov 14, 2017 •

edited

Loading