You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:10:08 UTC
[GitHub] [arrow] BryanCutler opened a new pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
BryanCutler opened a new pull request #10513:
URL: https://github.com/apache/arrow/pull/10513
Currently UnionVector and DenseUnionVector do not extend any base class, only implement the FieldVector interface. This change makes these vectors extend AbstractContainerVector, which is a subclass of BaseValueVector. This allows a the union vectors to be used in extension types as the underlying vector storage.
The current naming of child fields in the union vector ignores the original name and generates a new name based on type (and typeid). This has been changed to respect the original field name if given, and only if the name is empty it will generate a new name as before. Some current test add child Fields with empty names, so this preserves the original behavior.
Included tests to verify the added methods from AbstractContainerVector.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler closed pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
BryanCutler closed pull request #10513:
URL: https://github.com/apache/arrow/pull/10513
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-859790741
Ah you're right @liyafan82 , I must have mixed it up with `ValueVector`, so probably extension types won't work for struct and union types. I still think it's right for `UnionVector` to extend `AbstractContainerVector`, but probably not `BaseValueVector`. Maybe the constraint for `ExtensionTypeVector` should be relaxed to `ValueVector`? What do you and @lidavidm think?
cc @emkornfield
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-859416558
@BryanCutler Thanks for the PR.
According our current code base, it seems that `AbstractContainerVector` is not a subclass of `BaseValueVector`.
Does it prevent `UnionVector` from being used in extension types?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-859152849
https://issues.apache.org/jira/browse/ARROW-13044
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-860884676
Thanks @lidavidm , I made https://issues.apache.org/jira/browse/ARROW-13076 for ExtensionTypeVector to use ValueVector.
If this PR looks ok for union vectors, I'll follow up after to change the ExtensionTypeVector. That should enable structs and unions and I'll add some tests there also.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-861157094
merged to master
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler edited a comment on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
BryanCutler edited a comment on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-860884676
Thanks @lidavidm @liyafan82 , I made https://issues.apache.org/jira/browse/ARROW-13076 for ExtensionTypeVector to use ValueVector.
If this PR looks ok for union vectors, I'll follow up after to change the ExtensionTypeVector. That should enable structs and unions and I'll add some tests there also.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10513: ARROW-13044: [Java] Change UnionVector and DenseUnionVector to extend AbstractContainerVector
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10513:
URL: https://github.com/apache/arrow/pull/10513#issuecomment-859949559
I think that's fine, ValueVector is just the interface for BaseValueVector anyways? I admit I didn't look too closely. I don't remember why I specified the impl instead of the interface originally.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org