Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-13645 [Java]: Allow NullVectors to have distinct field names #10949

Closed
wants to merge 1 commit into from

Conversation

liyafan82
Copy link
Contributor

@liyafan82 liyafan82 commented Aug 17, 2021

As discussed in the ML (https://lists.apache.org/thread.html/r19aad8a34f63334d3fcd627106f69be13f689740b83930a4eecc17b3%40%3Cdev.arrow.apache.org%3E), the current implementation use hard-coded field names. This may cause some problems, for example, when reconstruct a usable schema from a list of FieldVectors.

So we resolve this problem by allowing NullVectors to have distinct field names.

This also makes the NullVector consistent with other types of vectors.

@github-actions
Copy link

@kiszk
Copy link
Member

kiszk commented Aug 18, 2021

LGTM
nit: The last line of the description may have a typo, This also makes the ZeroVector with other types of vectors. looks correct based on the change.

@liyafan82
Copy link
Contributor Author

LGTM
nit: The last line of the description may have a typo, This also makes the ZeroVector with other types of vectors. looks correct based on the change.

@kiszk Thanks for your feedback. I have updated the description accordingly.

@kiszk
Copy link
Member

kiszk commented Aug 23, 2021

I still see NullVector instead of ZeroVector. But, it is nit.

@liyafan82
Copy link
Contributor Author

I still see NullVector instead of ZeroVector. But, it is nit.

@kiszk Thanks for your careful review.
According to the current implementation, ZeroVector is a sub-class of NullVector, and the patch applies to both ZeroVector and NullVector. So I used the more general term (NullVector) here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants