laurentgo commented on code in PR #892:
URL: https://github.com/apache/arrow-java/pull/892#discussion_r2512499870


##########
vector/src/main/codegen/templates/PromotableWriter.java:
##########
@@ -325,6 +325,9 @@ protected boolean requiresArrowType(MinorType type) {
 
   @Override
   protected FieldWriter getWriter(MinorType type, ArrowType arrowType) {
+    if(type == MinorType.EXTENSIONTYPE) {

Review Comment:
   I'm not 100% sure of the exact interaction between those methods to confirm 
this is correct or not? For sure a comment in the code to explain would be nice.



##########
vector/src/main/codegen/templates/PromotableWriter.java:
##########
@@ -286,7 +286,7 @@ protected void setWriter(ValueVector v) {
         writer = new UnionWriter((UnionVector) vector, 
nullableStructWriterFactory);
         break;
       case EXTENSIONTYPE:
-        writer = new UnionExtensionWriter((ExtensionTypeVector) vector);
+        writer = ((ExtensionType) 
vector.getField().getType()).getNewFieldWriter(vector);

Review Comment:
   (nit) it seems casting should not be necessary because `getNewFieldWriter()` 
is defined as a `ArrowType` method, although I would be okay to only define it 
for `ExtensionType` for now?



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


Review Comment:
   This file is part of the 18.3.0 release, so removing it would be a breaking 
change. We could have a discussion if it okay or not. If it is, maybe we can be 
a bit more decisive on some other methods (like 
PromotableWriter#writeExtension(Object)) but otherwise, file need to be kept 
with a `@Deprecated` annotation



##########
vector/src/main/codegen/templates/PromotableWriter.java:
##########
@@ -540,18 +543,25 @@ public void writeLargeVarChar(String value) {
     getWriter(MinorType.LARGEVARCHAR).writeLargeVarChar(value);
   }
 
+  protected ArrowType lastExtensionType;
+
   @Override
   public void writeExtension(Object value) {

Review Comment:
   (design) should we deprecate this method? (since we now have 
`writeExtension(ExtensionHolder)`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to