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

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

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

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

        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

 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.

Based on changes from apache/arrow#41731. Added writer ExtensionWriter with 3 methods: - write method for writing values from Extension holders; - writeExtensionType method for writing values (arguments is Object because we don't know exact type); - addExtensionTypeFactory method - because exact vector and value type are unknown, user should create their own extension type vector, writer for it and ExtensionTypeFactory where it should map vector and writer.

lidavidm · 2025-04-03T04:37:29Z

Migrated to apache/arrow-java#697

Based on changes from apache/arrow#41731. ## What's Changed Added writer ExtensionWriter with 3 methods: - write method for writing values from Extension holders; - writeExtensionType method for writing values (arguments is Object because we don't know exact type); - addExtensionTypeFactory method - because the exact vector and value type are unknown, the user should create their own extension type vector, write for it, and ExtensionTypeFactory, which should map the vector and writer. Closes #87. Co-authored-by: Finn Völkel <[email protected]>

Based on changes from apache/arrow#41731. ## What's Changed Added writer ExtensionWriter with 3 methods: - write method for writing values from Extension holders; - writeExtensionType method for writing values (arguments is Object because we don't know exact type); - addExtensionTypeFactory method - because the exact vector and value type are unknown, the user should create their own extension type vector, write for it, and ExtensionTypeFactory, which should map the vector and writer. Closes apache#87. Co-authored-by: Finn Völkel <[email protected]>

Based on changes from apache#41731. Added writer ExtensionWriter with 3 methods: - write method for writing values from Extension holders; - writeExtensionType method for writing values (arguments is Object because we don't know exact type); - addExtensionTypeFactory method - because the exact vector and value type are unknown, the user should create their own extension type vector, write for it, and ExtensionTypeFactory, which should map the vector and writer. Closes dremio#87. Co-authored-by: Finn Völkel <[email protected]>

* GH-87: [Vector] Add ExtensionWriter (apache#697) Based on changes from apache#41731. Added writer ExtensionWriter with 3 methods: - write method for writing values from Extension holders; - writeExtensionType method for writing values (arguments is Object because we don't know exact type); - addExtensionTypeFactory method - because the exact vector and value type are unknown, the user should create their own extension type vector, write for it, and ExtensionTypeFactory, which should map the vector and writer. Closes #87. Co-authored-by: Finn Völkel <[email protected]> * just checking * Update Brewfile * revert * Fix for Cmake 3.xx installation * Fix for Cmake 3.xx installation * update thrift * update thrift url * update thrift url * Update github.yml * fix format --------- Co-authored-by: Finn Völkel <[email protected]>

Based on changes from apache/arrow#41731. ## What's Changed Added writer ExtensionWriter with 3 methods: - write method for writing values from Extension holders; - writeExtensionType method for writing values (arguments is Object because we don't know exact type); - addExtensionTypeFactory method - because the exact vector and value type are unknown, the user should create their own extension type vector, write for it, and ExtensionTypeFactory, which should map the vector and writer. Closes apache#87. Co-authored-by: Finn Völkel <[email protected]>

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

xxlaykxx mentioned this pull request Apr 1, 2025

GH-87: [Vector] Add ExtensionWriter apache/arrow-java#697

Merged

lidavidm closed this Apr 3, 2025

xxlaykxx mentioned this pull request Apr 23, 2025

GH-87: [Vector] Add ExtensionWriter (#697) dremio/arrow-java#4

Merged

xxlaykxx mentioned this pull request May 5, 2025

GH-87: [Vector] Add ExtensionWriter (#697) dremio/arrow#86

Merged

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

Uh oh!

Conversation

FiV0 commented May 20, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented May 20, 2024

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!

lidavidm commented May 21, 2024

Uh oh!

vibhatha commented May 21, 2024

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!

vibhatha left a comment

Choose a reason for hiding this comment

Uh oh!

FiV0 commented May 23, 2024

Uh oh!

lidavidm commented May 24, 2024

Uh oh!

FiV0 commented May 24, 2024

Uh oh!

lidavidm commented May 25, 2024

Uh oh!

github-actions bot commented Nov 26, 2024

Uh oh!

lidavidm commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FiV0 commented May 20, 2024 •

edited by github-actions bot

Loading