ARROW-633/634: [Java] Add FixedSizeBinary support in Java and integration tests (Updated) #1492

alphalfalfa · 2018-01-18T16:37:18Z

The original PR is at #1012. Due to the major refactoring last year, changes are big. So I created this separate PR for easier to review and will cancel the other one later.

Changes include:

Arrow-633: [Java] Add support for FixedWidthBinary type
Arrow-634: Add integration tests for FixedSizeBinary

alphalfalfa · 2018-01-18T16:38:54Z

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

 public abstract class BaseFixedWidthVector extends BaseValueVector
        implements FixedWidthVector, FieldVector, VectorDefinitionSetter {
-  private final byte typeWidth;
+  private final int typeWidth;


I have not changed the derived vector classes to use integer type. Should I?

I don't think it matters.

The TYPE_WIDTH const in the subclass is a static variable.

for other types, they can be static, for fixed size binary, it has to be non-static

Maybe if you can update BitVector since it currently casts down to a byte

alphalfalfa · 2018-01-18T16:41:55Z

@wesm @icexelloss

icexelloss · 2018-01-18T21:42:00Z

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

+    if (isSet(index) == 0) {
+      return null;
+    } else {
+      return get(index);


Expand this function to avoid duplicate check?

Sorry I didn't see the change. Did you push the change?

nvm. This looks good now.

icexelloss · 2018-01-18T21:50:44Z

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

+            if (byteWidth == 0) {
+              byteWidth = value.length;
+            } else if (byteWidth != value.length) {
+              throw new IOException("mismatch byte width (" + value.length + ") at index " + i + ", expecting " + byteWidth);


Does this mean the input is malformatted? If this is, then it probably shouldn't be IOException..

Changed to RuntimeException

icexelloss · 2018-01-18T21:50:57Z

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

+          }
+        }
+        if (count > 0 && byteWidth == 0) {
+          throw new IOException("could not determine the byte width of the vector because all elements are null");


changed to RuntimeException

icexelloss · 2018-01-18T21:52:20Z

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

+          values.add(value);
+          if (value.length > 0) {
+            if (byteWidth == 0) {
+              byteWidth = value.length;


Can you read the byteWidth from the schema rather than interpret it here?

byteWidth info is now collected from the vector and passed in

Sorry I am not sure I understand.. Why don't we use the byteWidth in schema to inform the expected length here?

icexelloss · 2018-01-18T21:53:38Z

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

+
+        ArrowBuf buf = allocator.buffer(byteWidth * count);
+        for (byte[] value : values) {
+          buf.writeBytes(value.length == 0? new byte[byteWidth] : value);


Why can there be empty values?

For nullable vectors, when the value is null, the corresponding JSON field is empty.

icexelloss · 2018-01-18T21:56:18Z

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

+   * @param index   position of the element to set
+   * @param holder  holder that carries data buffer.
+   */
+  public void set(int index, NullableFixedSizeBinaryHolder holder) {


Should we call this FixedSizeBinaryHolder instead?

There are both Holders and NullableHolders.

icexelloss · 2018-01-18T21:56:43Z

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

+      to.copyFromSafe(fromIndex, toIndex, FixedSizeBinaryVector.this);
+    }
+  }
+}


Need a newline

icexelloss · 2018-01-18T22:13:16Z

High level looks good to me. Need to come back and look in more detail.

…ck of isSet()

alphalfalfa · 2018-01-23T15:40:11Z

Anybody knows what should I do with the check failure:

The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends --force-yes install gcc-4.9 g++-4.9 gdb ccache valgrind libboost-dev libboost-filesystem-dev libboost-system-dev libjemalloc-dev gtk-doc-tools autoconf-archive libgirepository1.0-dev" failed and exited with 100 during .

Also, more comments on this PR?

icexelloss · 2018-01-23T15:54:20Z

@siddharthteotia or @BryanCutler Could one of you please also take a look? Thanks!

wesm · 2018-01-23T16:07:18Z

@alphalfalfa we've been experiencing a lot of apt flakiness in Travis CI. Someone should escalate it with the support system to see what's going on

wesm · 2018-01-23T16:08:18Z

I just restarted the failing job

icexelloss · 2018-01-23T16:11:24Z

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

    BufferReader INT8 = new BufferReader() {
      @Override
-      protected ArrowBuf read(BufferAllocator allocator, int count) throws IOException {
+      protected ArrowBuf read(BufferAllocator allocator, int count, int byteWidth) throws IOException {


Hmm... I am not sure about this. Having byteWidth in every reader seems weird because most don't need it. Can you put make a member variable of fixed size binary reader?

icexelloss · 2018-01-23T16:14:26Z

integration/integration_test.py

+    def _get_buffers(self):
+        data = []
+        for i, v in enumerate(self.values):
+            data.append(self._encode_value(v if self.is_valid[i] else ""))


This seems strange. Why are we using empty string to represent null value?

We can probably just use any value for null instead of empty string. It should make the logic cleaner. (less if statements)

oh... I put empty strings as null. sure, i can put some arbitrary strings

made the changes and simplified the logic inside JsonFileReader

icexelloss · 2018-01-24T18:19:47Z

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

          if (bufferType.equals(DATA) && (vector.getMinorType() == Types.MinorType.VARCHAR ||
                  vector.getMinorType() == Types.MinorType.VARBINARY)) {
-            writeValueToGenerator(bufferType, vectorBuffer, vectorBuffers.get(v-1), vector, i, scale);
+            writeValueToGenerator(bufferType, vectorBuffer, vectorBuffers.get(v-1), vector, i, scale, byteWidth);


This method doesn't need to take scale and byteWidth. It can figure this out from vector.

good catch, modified.

…Writer.java

wesm · 2018-01-29T22:33:58Z

@siddharthteotia @jacques-n could we get some more eyes on this? Thanks!

BryanCutler

Thanks @alphalfalfa , looks pretty good! I just had some minor suggestions

BryanCutler · 2018-01-30T23:08:21Z

integration/integration_test.py

+class FixedSizeBinaryType(PrimitiveType):
+
+    def __init__(self, name, byte_width, nullable=True):
+        PrimitiveType.__init__(self, name, nullable)


minor: I think super(..).__init__(..) is a little better

BryanCutler · 2018-01-30T23:20:50Z

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

 public abstract class BaseFixedWidthVector extends BaseValueVector
        implements FixedWidthVector, FieldVector, VectorDefinitionSetter {
-  private final byte typeWidth;
+  private final int typeWidth;


Maybe if you can update BitVector since it currently casts down to a byte

BryanCutler · 2018-01-30T23:34:21Z

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


+      @Override
+      public TypeLayout visit(FixedSizeBinary type) {
+        return newFixedWidthTypeLayout(BufferLayout.dataBuffer(type.getByteWidth() * 8));


could you just do?

return newFixedWidthTypeLayout(new BufferLayout(BufferType.DATA, type.getByteWidth() * 8)

Then you don't need to BufferLayout dataBuffer(int typeBitWidth)
and then keep the

BryanCutler · 2018-01-30T23:46:28Z

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

        return VALUES_128;
      default:
-        throw new IllegalArgumentException("only 8, 16, 32, or 64 bits supported");
+        return new BufferLayout(BufferType.DATA, typeBitWidth);


I think function is really "get data buffer layout for types that support certain bit widths" - so it might be beneficial to keep the exception, although it looks like it should be updated to include 128. I'm not sure how exactly the exception would be thrown, but if somehow a buffer layout for an Int with 30 bit width was created, this would catch it.

reverted and added 128 bits to the exception message

BryanCutler · 2018-01-30T23:48:47Z

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

+          values.add(value);
+        }
+
+        int byteWidth = count > 0? values.get(0).length : 0;


nit: add space before ?

…ng dataBuffer() method and some other changes based on PR comments

icexelloss · 2018-02-02T16:30:53Z

@siddharthteotia Do you think we can merge this? LGTM but want to double check with you.

jacques-n · 2018-02-07T15:59:51Z

It seems like we are missing support in the ComplexWriter and FieldReader interfaces. I think that should be included in introduction of these things. The patch here looks fine but I think we should include that before releasing this feature (either in this patch or a follow-up).

jacques-n · 2018-02-07T16:01:45Z

Per my comment, I'm +1 on the spirit on these changes but think the patch (or feature) is somewhat incomplete

alphalfalfa · 2018-02-07T20:05:46Z

@jacques-n, to make sure that I understand the question properly, do you mean there should be proper interfaces defined for FixedSizeBinary type inside AbstractFieldWriter and AbstractFieldReader?
Currently, AbstractFieldWriter has:
public void writeFixedSizeBinary(ArrowBuf buffer)
AbstractFieldReaderWriter has:
public byte[] readByteArray()
What would be the desired interfaces to add?

I would prefer to add the necessary fix in a separate PR as this one has grown too big if it is OK.

jacques-n · 2018-02-07T22:25:42Z

You're right. I forgot that the code is autogenerated and was looking for it here. I'm +1 on this PR.

wesm · 2018-02-07T22:34:27Z

thanks all, please open any follow-up JIRAs

…tion tests (Updated) The original PR is at apache#1012. Due to the major refactoring last year, changes are big. So I created this separate PR for easier to review and will cancel the other one later. Changes include: * Arrow-633: [Java] Add support for FixedWidthBinary type * Arrow-634: Add integration tests for FixedSizeBinary Author: Jingyuan Wang <jingyuan@live.com> Author: jingyuan <jingyuan.nt@gmail.com> Closes apache#1492 from alphalfalfa/updated-arrow-633-634 and squashes the following commits: c994a19 [jingyuan] create data buffer layout for FixedSizeBinary directly instead of using dataBuffer() method and some other changes based on PR comments 873c5b2 [jingyuan] do not pass scale or byteWidth into writeValueToGenerator in JsonFileWriter.java 4fbb67d [jingyuan] put arbitrary values for FixSizeBinary nulls 0c0015d [Jingyuan Wang] add a new line a8a9553 [Jingyuan Wang] Pass byteWidth info for FixedSizeBinary type in JsonFileReader 1a64c5c [Jingyuan Wang] expand get() method inside getObject() method to remove duplicate check of isSet() 071fb25 [Jingyuan Wang] ARROW-633/634: Add FixedSizeBinary support in Java and integration tests

ARROW-633/634: Add FixedSizeBinary support in Java and integration tests

071fb25

alphalfalfa commented Jan 18, 2018

View reviewed changes

icexelloss reviewed Jan 18, 2018

View reviewed changes

alphalfalfa added 3 commits January 19, 2018 14:07

expand get() method inside getObject() method to remove duplicate che…

1a64c5c

…ck of isSet()

Pass byteWidth info for FixedSizeBinary type in JsonFileReader

a8a9553

add a new line

0c0015d

icexelloss reviewed Jan 23, 2018

View reviewed changes

put arbitrary values for FixSizeBinary nulls

4fbb67d

icexelloss reviewed Jan 24, 2018

View reviewed changes

do not pass scale or byteWidth into writeValueToGenerator in JsonFile…

873c5b2

…Writer.java

BryanCutler reviewed Jan 31, 2018

View reviewed changes

create data buffer layout for FixedSizeBinary directly instead of usi…

c994a19

…ng dataBuffer() method and some other changes based on PR comments

wesm changed the title ~~ARROW-633/634: Add FixedSizeBinary support in Java and integration tests (Updated)~~ ARROW-633/634: [Java] Add FixedSizeBinary support in Java and integration tests (Updated) Feb 5, 2018

wesm closed this in f69e9db Feb 7, 2018

asfimport mentioned this pull request Feb 7, 2018

[Java] Add support for FixedSizeBinary type #15364

Closed

ARROW-633/634: [Java] Add FixedSizeBinary support in Java and integration tests (Updated) #1492

ARROW-633/634: [Java] Add FixedSizeBinary support in Java and integration tests (Updated) #1492

Uh oh!

Conversation

alphalfalfa commented Jan 18, 2018

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!

alphalfalfa commented Jan 18, 2018

Uh oh!

icexelloss Jan 18, 2018 • 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 Jan 18, 2018 • 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!

icexelloss commented Jan 18, 2018

Uh oh!

alphalfalfa commented Jan 23, 2018

Uh oh!

icexelloss commented Jan 23, 2018

Uh oh!

wesm commented Jan 23, 2018

Uh oh!

wesm commented Jan 23, 2018

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!

alphalfalfa Jan 23, 2018 • 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!

icexelloss Jan 18, 2018 •

edited

Loading

icexelloss Jan 18, 2018 •

edited

Loading

alphalfalfa Jan 23, 2018 •

edited

Loading