martin-traverse commented on PR #638:
URL: https://github.com/apache/arrow-java/pull/638#issuecomment-2749052296

   Hi @lidavidm , @wgtmac  - Have pushed another update, here is a quick 
summary:
   
   * I have been through all the comments above and responded. Please do come 
back on any you think need further attention!
   
   * I have added tests for both schema and data production. Tests cover 
primitive types, logical types and containers  of primitive and logical types. 
Some edges cases (max int values, UTF 8 text) are included.
   
   * Tests do not include nested containers (e.g. list of lists), these may 
work but I've not tried it. Negative test cases are also not included (i.e.  
invalid / illegal / badly initialised data structures).
   
   * Dictionary encoding not supported yet (we could push this to a second PR)
   
   * Nullable types. I have [type, null] for nullable types instead of [null, 
type]. There was a comment on this. The current reader framework won't allow 
the latter, so we can't round-trip if we do it that way, but if you think it is 
preferable we could do it anyway now we have full coverage on the producers. 
Anyway I think a separate PR is needed to address issues in the consumers. Lmk 
what you'd like to do on this.
   
   * Union data. Union schema production is working fine, but union data I 
really struggled with. Is there an issue with the field writers? I don't see 
how [this 
code](https://github.com/apache/arrow-java/blob/48a20ba91605ebb44df845671782acd8f869bd4b/vector/src/main/codegen/templates/DenseUnionWriter.java#L55)
 is ever going to work, since writers are not alll initialized. Also, per my 
understanding, the semantics of UnionWriter and DenseUnionWriter seem like 
they're the wrong way around. Fairly sure my understanding is the problem but I 
don't see where! Can you point me to any example code that populates a 
UnionVector and/or DenseUnionVector?
   
   Apologies for the delay, lots else going on as usual but I think this is 
getting closer now. If you let me know what else you'd like to see for this to 
be considered ready, I'll try to turn it around as quickly as I can.


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