GH-41728:[Java][Vector] Add ExtensionWriter and fix StructVector construction with ExtensionTypes #41731

FiV0 · 2024-05-20T11:45:00Z

Rationale for this change

For me personally I mainly want to fix apache/arrow-java#87, but this change also includes some larger changes to the writer interface. This will likely include some design work and some back and forth as I can't make these decisions.

What changes are included in this PR?

A commit that extends the writer interfaces with an ExtensionWriter as well as dealing with the MinorType.EXTENSIONTYPE in various places. The ExtensionWriter was left rather skeleton like as this needs some design decisions to be made before implementing the concrete details. I will leave some comments throughout the code of where I think stuff needs to be decided.

This next bit are my 2cents (hence taken with a grain of salt) of how I would do things as I have the impression that one will eventually hit a wall with the current writer interfaces. For example the PromotableWriter currently makes the assumption that the parent vector is a struct or a list vector, making it difficult to extend this to composite extension types.
I would just create one single writer interface (VectorWriter or whatever you want to call) implemented by all concrete writers. If a writer doesn't support an operation just keep it abstract or throw an UOE.

Are these changes tested?

There are two tests that test the issue of apache/arrow-java#87. I have not tested (because of the lack of a concrete implementation) the ExtensionWriter interface.

Are there any user-facing changes?

Yes. ExtensionWriter will be exposed for the first time. This is only an additive change and shouldn't break any existing API.

GitHub Issue: [Java] StructVector throws with ExtensionType arrow-java#87
GitHub Issue: ARROW-212: Change contract of PrimitiveArray to reflect its abstractness #87

github-actions · 2024-05-20T11:45:25Z

⚠️ GitHub issue apache/arrow-java#87 has been automatically assigned in GitHub to PR creator.

FiV0 · 2024-05-20T11:47:05Z

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

+import org.apache.arrow.vector.ExtensionTypeVector;
+import org.apache.arrow.vector.types.pojo.Field;
+
+public class UnionExtensionWriter extends AbstractFieldWriter {


This class is currently more skeleton like as the concrete details of the ExtensionWriter interface will need to be nailed down.

FiV0 · 2024-05-20T11:53:04Z

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

@@ -61,20 +61,25 @@ public interface StructWriter extends BaseWriter {



This is where some design decisions have to be made of how to actually expose the ExtensionWriter to the user and how the user should actually extend some ExtensionWriter.

As mentioned above I would make this one interface BaswWriter exposing all the possible writers and leaving them abstract in the concrete implementations.

This sounds reasonable to me.

FiV0 · 2024-05-20T11:56:51Z

java/vector/src/test/java/org/apache/arrow/vector/types/pojo/TestExtensionType.java

@@ -342,6 +347,34 @@ public void set(int index, UUID uuid) {
      bb.putLong(uuid.getLeastSignificantBits());
      getUnderlyingVector().set(index, bb.array());
    }
+
+    @Override
+    public TransferPair makeTransferPair(ValueVector to) {


This TransferPair is likely a very common pattern for extension types, so one could create a concrete ExtensionWriter which actually implements this and something like UuidWriter could inherit from.

We do something similar for our writers: https://github.com/xtdb/xtdb/blob/e9a15ee12244367c1d3d1a75c00e4f5c0e10008f/core/src/main/kotlin/xtdb/vector/extensions/XtExtensionVector.kt#L30-L40

Seems to be fine, let's get this one working and further evaluate.

lidavidm · 2024-05-21T06:01:55Z

CC @vibhatha @danepitkin

vibhatha · 2024-05-21T10:07:32Z

@FiV0 I will take a look at this PR later today or earlier tomorrow.

vibhatha · 2024-05-22T12:05:39Z

java/vector/src/test/java/org/apache/arrow/vector/types/pojo/TestExtensionType.java

@@ -85,21 +88,21 @@ public void roundtripUuid() throws IOException {
          final ArrowFileReader reader = new ArrowFileReader(channel, allocator)) {
        reader.loadNextBatch();
        final VectorSchemaRoot readerRoot = reader.getVectorSchemaRoot();
-        Assert.assertEquals(root.getSchema(), readerRoot.getSchema());
+        assertEquals(root.getSchema(), readerRoot.getSchema());


Just a thought: since we are updating test cases, shall we use JUnit5? juputer.api.?

vibhatha · 2024-05-22T12:16:44Z

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

@@ -17,12 +17,17 @@

 package org.apache.arrow.vector;

-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;


could we use JUnit5?

vibhatha · 2024-05-22T12:23:57Z

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

+    final StructVector toVector = (StructVector) tp.getTo();
+    assertEquals(s1.getField(), toVector.getField());
+    assertEquals(s1.getField().getChildren().get(0), toVector.getField().getChildren().get(0));
+    // also fails but probably another issue


Yes, I also debugged the code. It seems the copy didn't work properly. I guess you're still not finished with this work, right?

vibhatha

The overall idea looks fine to me, but need to run another pass.

And the added test case seems to need some work as we don't get the expected results from the transferPair AFAIU.

FiV0 · 2024-05-23T10:20:02Z

I think the main part of moving this forward is the design work on the writer or more specifically ExtensionWriter interface. I think it would be good to have some rough idea of how this will look in the future for the user, as otherwise we will need to change this again when we realize that the current API doesn't work. For example.

How do you expect people to write to a UuidVector?
How do you expect people to write to a composite extension vector ( for example a SetVector as an extension to a ListVector)?
How do you expect people to write to a Union with an extension leg? How should a union with two extensions legs of different type be dealt with? etc..

I think all of this should have a clear answer before moving forward with this proposal. Maybe have a look at what other language implementations do in this case.

lidavidm · 2024-05-24T03:39:49Z

They don't really have answers for this.

Does it work to just ask users to build the underlying type directly for now?

FiV0 · 2024-05-24T13:57:01Z

@lidavidm Ok, I will then create a Test for ExtensionWriter (when I find the time) with the cases outlined above, so that we have at least some idea of what it will look like.

lidavidm · 2024-05-25T01:10:24Z

Thanks @FiV0!

github-actions · 2024-11-26T19:14:14Z

⚠️ GitHub issue #41728 has no components, please add labels for components.

FiV0 requested a review from lidavidm as a code owner May 20, 2024 11:45

github-actions bot added Component: Java awaiting review Awaiting review labels May 20, 2024

FiV0 commented May 20, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 20, 2024

FiV0 force-pushed the structs-and-extensions branch from 934466e to b4bda84 Compare May 20, 2024 11:48

FiV0 commented May 20, 2024

View reviewed changes

FiV0 force-pushed the structs-and-extensions branch from b4bda84 to f4f0e4d Compare May 21, 2024 10:06

FiV0 added 2 commits May 21, 2024 12:08

Adding ExtensionWriter and fix for #41728

01cdcc3

Adding test for #41728

7cb7fd5

FiV0 force-pushed the structs-and-extensions branch from f4f0e4d to 7cb7fd5 Compare May 21, 2024 14:25

vibhatha reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41728:[Java][Vector] Add ExtensionWriter and fix StructVector construction with ExtensionTypes #41731

GH-41728:[Java][Vector] Add ExtensionWriter and fix StructVector construction with ExtensionTypes #41731

FiV0 commented May 20, 2024 •

edited by github-actions bot

Loading

github-actions bot commented May 20, 2024

FiV0 May 20, 2024

FiV0 May 20, 2024

vibhatha May 22, 2024

FiV0 May 20, 2024

vibhatha May 22, 2024

lidavidm commented May 21, 2024

vibhatha commented May 21, 2024

vibhatha May 22, 2024

vibhatha May 22, 2024

vibhatha May 22, 2024

vibhatha left a comment

FiV0 commented May 23, 2024

lidavidm commented May 24, 2024

FiV0 commented May 24, 2024

lidavidm commented May 25, 2024

github-actions bot commented Nov 26, 2024

		@@ -61,20 +61,25 @@ public interface StructWriter extends BaseWriter {

GH-41728:[Java][Vector] Add ExtensionWriter and fix StructVector construction with ExtensionTypes #41731

Are you sure you want to change the base?

GH-41728:[Java][Vector] Add ExtensionWriter and fix StructVector construction with ExtensionTypes #41731

Conversation

FiV0 commented May 20, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented May 21, 2024

vibhatha commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha left a comment

Choose a reason for hiding this comment

FiV0 commented May 23, 2024

lidavidm commented May 24, 2024

FiV0 commented May 24, 2024

lidavidm commented May 25, 2024

github-actions bot commented Nov 26, 2024

FiV0 commented May 20, 2024 •

edited by github-actions bot

Loading