-
Notifications
You must be signed in to change notification settings - Fork 92
GH-586: Override fixedSizeBinary method for UnionMapWriter #885
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Hm, looks like I don't have enough permissions to add labels. Could the maintainers add the |
|
Hi @nbauernfeind! It's my attempt to address the issue you opened. Could you please take a look? Is it what you had in mind for it? |
|
@axreldable sure. I will. FYI we are working on fixing the CI right now. Your PR will probably need a rebase as soon as CI is green again. I will keep you posted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it technically works but I find it very confusing to recycle the test, especially since the map value was always BigInt before. I'd rather see a new test that exercises both the key/value paths explicitly and is explicitly marked as a regression test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidavidm , thank you for looking into it! Sorry for the confusion. Let me introduce a separate unit test.
--
Could you please clarify what do you mean by
explicitly marked as a regression test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** Regression test for https://github.com/apache/arrow-java/issues/586 */
@Test
void mapFixedSizeBinary() {
// ...
What's Changed
UnionMapWriterwill null out the entire map struct entry instead of setting the value to null in:This PR overrides the
fixedSizeBinarymethod for theUnionMapWriter, resolving it.Closes #586.