GH-87: [Vector] Add ExtensionWriter #697

xxlaykxx · 2025-04-01T08:14:57Z

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. 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.

xxlaykxx · 2025-04-01T08:25:20Z

@lidavidm, please take a look if this approach can be used

lidavidm

Can we preserve the original committer's information if possible so they can get the credit for the original work? (A Co-authored-by tag in the PR should suffice I think)

Is there some way we could have a type-safe design instead of just Object everywhere?

lidavidm · 2025-04-03T04:32:19Z

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

+
+import org.apache.arrow.vector.ExtensionTypeVector;
+
+public interface ExtensionTypeWriterFactory<T extends AbstractFieldWriter> {


Can we document this?

Does the type bound need to be AbstractFieldWriter or can it just be FieldWriter (the interface)?

I chose AbstractFieldWriter just to make sure that the user will use some specific implementation of writer, but in general yes - it could be FieldWriter

IMO abstract implementation classes should not go in generic bounds - it should be the interface type

vector/src/main/java/org/apache/arrow/vector/holders/ExtensionHolder.java

lidavidm · 2025-04-03T04:34:31Z

vector/src/test/java/org/apache/arrow/vector/complex/impl/TestPromotableWriter.java

+      this.vector = vector;
+    }
+
+    @Override


Is there a way we can provide default implementations for these functions to reduce the boilerplate?

lidavidm · 2025-04-03T04:35:14Z

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

We may want to just pull the UuidType into a toplevel class now that it's being used by multiple tests.

xxlaykxx · 2025-04-04T08:26:19Z

Can we preserve the original committer's information if possible so they can get the credit for the original work? (A Co-authored-by tag in the PR should suffice I think)

Is there some way we could have a type-safe design instead of just Object everywhere?

Because the type is unknown, we could allow only Holder impl for writing. Is this acceptable?

lidavidm · 2025-04-04T08:42:07Z

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

+
+public class TestUuidVector {
+
+  public static class UuidType extends ExtensionType {


What I meant was, can we just make this a toplevel class? Not a nested class?

lidavidm · 2025-04-04T08:42:20Z

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

+    }
+  }
+
+  public static class UuidVector extends ExtensionTypeVector<FixedSizeBinaryVector>


lidavidm · 2025-04-04T08:42:43Z

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

+import org.apache.arrow.vector.types.pojo.ArrowType.ExtensionType;
+import org.apache.arrow.vector.util.TransferPair;
+
+public class TestUuidVector {


In fact why is this class here at all? All it does is wrap the two inner classes

lidavidm · 2025-04-04T08:44:56Z

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

+    void writeNull();
+    <T extends ExtensionHolder> void write(T var1);
+    void writeExtensionType(Object var1);
+    <T extends ExtensionTypeWriterFactory> void addExtensionTypeFactory(T var1);


While the existing code is short on docstrings, can we add docstrings for new code going forward? In particular it's not clear what addExtensionTypeFactory is or how to use it

Are the generic bounds actually useful here? Why can't it just be ExtensionTypeWriterFactory var1?

Can we replace var1 with meaningful parameter names?

lidavidm · 2025-04-04T08:46:18Z

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


+  public interface ExtensionWriter extends BaseWriter {
+    void writeNull();
+    <T extends ExtensionHolder> void write(T var1);


Same here - why not just void write(ExtensionHolder value)?

lidavidm · 2025-04-04T08:49:23Z

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

+
+import org.apache.arrow.vector.ExtensionTypeVector;
+
+public interface ExtensionTypeWriterFactory<T extends AbstractFieldWriter> {


IMO abstract implementation classes should not go in generic bounds - it should be the interface type

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

lidavidm · 2025-04-04T08:51:20Z

vector/src/test/java/org/apache/arrow/vector/complex/impl/TestPromotableWriter.java

I don't see any test of using ExtensionHolder?

lidavidm

Looking good, just some final naming questions

lidavidm · 2025-04-08T06:40:09Z

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

+    void writeNull();
+
+    /**
+     * Writes vlaue from the given extension holder.


Suggested change

* Writes vlaue from the given extension holder.

* Writes value from the given extension holder.

lidavidm · 2025-04-08T06:42:00Z

vector/src/test/java/org/apache/arrow/vector/complex/impl/UuidWriterImpl.java

+
+  @Override
+  public void write(ExtensionHolder holder) {
+    if (holder instanceof UuidHolder) {


Either delete the check to be consistent with writeExtensionType or explicitly error if the holder is the wrong type

lidavidm · 2025-04-08T06:42:19Z

vector/src/test/java/org/apache/arrow/vector/complex/impl/UuidWriterImpl.java

+import org.apache.arrow.vector.holder.UuidHolder;
+import org.apache.arrow.vector.holders.ExtensionHolder;
+
+public class UuidWriterImpl extends AbstractExtensionTypeWriter {


Suggested change

public class UuidWriterImpl extends AbstractExtensionTypeWriter {

public class UuidWriterImpl extends AbstractExtensionTypeWriter<UuidVector> {

lidavidm · 2025-04-08T06:42:27Z

vector/src/test/java/org/apache/arrow/vector/complex/impl/UuidWriterImpl.java

+    ByteBuffer bb = ByteBuffer.allocate(16);
+    bb.putLong(uuid.getMostSignificantBits());
+    bb.putLong(uuid.getLeastSignificantBits());
+    ((UuidVector) this.vector).setSafe(this.idx(), bb.array());


Suggested change

((UuidVector) this.vector).setSafe(this.idx(), bb.array());

vector.setSafe(idx(), bb.array());

lidavidm · 2025-04-08T06:43:04Z

vector/src/test/java/org/apache/arrow/vector/complex/impl/UuidWriterImpl.java

+      ((UuidVector) this.vector).setSafe(this.idx(), uuidHolder.value);
+      this.vector.setValueCount(this.idx() + 1);


Suggested change

((UuidVector) this.vector).setSafe(this.idx(), uuidHolder.value);

this.vector.setValueCount(this.idx() + 1);

vector.setSafe(idx(), uuidHolder.value);

vector.setValueCount(idx() + 1);

lidavidm · 2025-04-08T06:51:29Z

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

+     *
+     * @param value the extension type value to write
+     */
+    void writeExtensionType(Object value);


This is a nit but now I'm thinking it should be just writeExtension to parallel writeVarChar

lidavidm · 2025-04-08T06:51:48Z

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

+     *
+     * @param factory the extension type factory to add
+     */
+    void addExtensionTypeFactory(ExtensionTypeWriterFactory factory);


Suggested change

void addExtensionTypeFactory(ExtensionTypeWriterFactory factory);

void addExtensionTypeWriterFactory(ExtensionTypeWriterFactory factory);

for consistency as well

lidavidm · 2025-04-08T06:53:20Z

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

+
+  @Override
+  protected int idx() {
+    return super.idx();


Hmm, is the only reason we override here to make it protected instead of package-private? (Should we just use getPosition() instead as it is already public and does the same thing?)

yep, makes sense - logic with idx() method added for all other writers.

lidavidm

LGTM, but please see the pre-commit failures

lidavidm · 2025-04-09T12:19:58Z

Thanks @xxlaykxx @FiV0!

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]>

apacheGH-87: [Vector] Add ExtensionWriter (apache#697)

* apacheGH-87: [Vector] Add ExtensionWriter (apache#697) missed file

* apacheGH-87: [Vector] Add ExtensionWriter (apache#697) missed file * apacheGH-87: [Vector] Add ExtensionWriter (apache#697) missed file * apacheGH-87: [Vector] Add ExtensionWriter (apache#697) updated UnionListWriter.

xxlaykxx requested review from lidavidm, laurentgo, wgtmac and jbonofre as code owners April 1, 2025 08:14

github-actions bot added the breaking-change label Apr 1, 2025

This comment has been minimized.

Sign in to view

lidavidm added enhancement PRs that add or improve features. and removed breaking-change labels Apr 3, 2025

github-actions bot added this to the 18.3.0 milestone Apr 3, 2025

lidavidm reviewed Apr 3, 2025

View reviewed changes

lidavidm mentioned this pull request Apr 3, 2025

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

Closed

lidavidm changed the title ~~GH-87: [Java][Vector] Add ExtensionWriter for List/Struct.~~ GH-87: [Java][Vector] Add ExtensionWriter Apr 3, 2025

lidavidm changed the title ~~GH-87: [Java][Vector] Add ExtensionWriter~~ GH-87: [Vector] Add ExtensionWriter Apr 3, 2025

xxlaykxx and others added 3 commits April 3, 2025 19:36

apacheGH-87: fixed build

9f96325

apacheGH-87: fixed build

867c797

Merge branch 'apache:main' into apacheGH-87

fc08192

xxlaykxx changed the title ~~GH-87: [Vector] Add ExtensionWriter~~ GH-87 [Vector] Add ExtensionWriter Apr 4, 2025

xxlaykxx changed the title ~~GH-87 [Vector] Add ExtensionWriter~~ GH-87: [Vector] Add ExtensionWriter Apr 4, 2025

lidavidm reviewed Apr 4, 2025

View reviewed changes

apacheGH-87: fixed comments

4ab857c

lidavidm reviewed Apr 8, 2025

View reviewed changes

xxlaykxx added 2 commits April 9, 2025 09:26

apacheGH-87: fixed comments

3e88e76

apacheGH-87: removed unneeded override

4df283a

xxlaykxx requested a review from lidavidm April 9, 2025 06:43

lidavidm reviewed Apr 9, 2025

View reviewed changes

xxlaykxx added 2 commits April 9, 2025 14:24

apacheGH-87: fixed build

9c8a022

apacheGH-87: fixed build

f8799ea

lidavidm merged commit 437612c into apache:main Apr 9, 2025
25 of 26 checks passed

xxlaykxx added a commit to dremio/arrow-java that referenced this pull request Apr 29, 2025

Merge pull request #4 from dremio/extension_writer_dremio_26.1_19.0.1

337eac2

apacheGH-87: [Vector] Add ExtensionWriter (apache#697)

lriggs pushed a commit to lriggs/arrow-java that referenced this pull request Jul 14, 2025

apacheGH-87: [Vector] Add ExtensionWriter (apache#697) (apache#89)

654ced6

* apacheGH-87: [Vector] Add ExtensionWriter (apache#697) missed file


		import org.apache.arrow.vector.ExtensionTypeVector;

		public interface ExtensionTypeWriterFactory<T extends AbstractFieldWriter> {


		public class TestUuidVector {

		public static class UuidType extends ExtensionType {

	* Writes vlaue from the given extension holder.
	* Writes value from the given extension holder.

	public class UuidWriterImpl extends AbstractExtensionTypeWriter {
	public class UuidWriterImpl extends AbstractExtensionTypeWriter<UuidVector> {

	((UuidVector) this.vector).setSafe(this.idx(), bb.array());
	vector.setSafe(idx(), bb.array());

		((UuidVector) this.vector).setSafe(this.idx(), uuidHolder.value);
		this.vector.setValueCount(this.idx() + 1);

	void addExtensionTypeFactory(ExtensionTypeWriterFactory factory);
	void addExtensionTypeWriterFactory(ExtensionTypeWriterFactory factory);

GH-87: [Vector] Add ExtensionWriter #697

GH-87: [Vector] Add ExtensionWriter #697

Uh oh!

Conversation

xxlaykxx commented Apr 1, 2025 • edited by lidavidm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's Changed

Uh oh!

This comment has been minimized.

xxlaykxx commented Apr 1, 2025

Uh oh!

lidavidm 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xxlaykxx commented Apr 4, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm 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!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lidavidm commented Apr 9, 2025

Uh oh!

Uh oh!

xxlaykxx commented Apr 1, 2025 •

edited by lidavidm

Loading