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-1815: [Java] Rename MapVector to StructVector #1490

Closed

Conversation

BryanCutler
Copy link
Member

This renames the MapVector to StructVector and NullableMapVector to NullableStructVector. Related classes have also been renamed as well as APIs making reference to "Map" meaning a struct. The MinorType.Map is also changed to MinorType.STRUCT.

@BryanCutler
Copy link
Member Author

@siddharthteotia this was pretty much straightforward renaming, so hopefully it is not too disruptive to you downstream. Let me know if I can help run Dremio tests if needed. cc @icexelloss

@icexelloss
Copy link
Contributor

@BryanCutler Are these the final names for the vectors? I think we were thinking about calling it StructVector instead of NullableStructVector but forgot how the discussion ended.

@BryanCutler
Copy link
Member Author

Are these the final names for the vectors? I think we were thinking about calling it StructVector instead of NullableStructVector but forgot how the discussion ended.

I think at one point we agreed that we would eventually merge the 2 and just have StructVector, but our most recent discussion was just about changing the name, so I kept it at that in order not to be too disruptive. If @siddharthteotia is ok with merging the 2 now, I can do that now or make a followup PR.

@BryanCutler
Copy link
Member Author

BryanCutler commented Jan 18, 2018

I believe there was some issues with runtime code optimizations downstream in Dremio that would need to be verified before merging the 2.

@icexelloss
Copy link
Contributor

Aha I see. Sounds good to me. @jacques-n I think the last discussion was left at

https://docs.google.com/document/d/1n4qjO20wZyS7wSpISgYdIVuD22zstLgP-7gXxS5_7_E/edit?disco=AAAABdbHcQQ

Do you think we can proceed with this patch? Or do sth different?

@wesm
Copy link
Member

wesm commented Jan 19, 2018

I think @jacques-n is on vacation this week, but we can wait for @siddharthteotia to advise

@siddharthteotia
Copy link
Contributor

I am fine with the name changes of MapVector to StructVector. However, I would request to keep the NullableStructVector as of now since there are potential impacts on reader/writer I believe.

I would like to patiently rebase Dremio on Arrow master -- we are yet to the changes w.r.t removal of non-nullable vectors and renaming of other vectors. Once that is done, we can hopefully remove NullableStructVector too.

@icexelloss
Copy link
Contributor

Hey folks, where do we stand on this? @siddharthteotia are we ok with the changes to MINOR type and reader/writer interface?

@jacques-n
Copy link
Contributor

It seems like we should be changing from NullableMapVector => StructVector and MapVector => NonNullableStructVector to be consistent with other names, no? Or do we discuss that as a follow along change?

@BryanCutler
Copy link
Member Author

I made the above name changes to the vector classes, so we now have StructVector and NonNullableStructVector. I looked into changing the reader/writer apis to match (currently we have SingleStructWriter and NullableStructWriter) and it gets a little messy because there is already an interface named StructWriter. So I left those as is and maybe they could be looked at whenever we end up merging the 2 vector classes. What do you think about this @jacques-n @siddharthteotia @icexelloss ?

@jacques-n
Copy link
Contributor

LGTM +1

@wesm
Copy link
Member

wesm commented Feb 15, 2018

Cool, will merge. Thanks all!

@wesm wesm closed this in 78152f1 Feb 15, 2018
@BryanCutler
Copy link
Member Author

Thanks @jacques-n and @wesm!

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

Successfully merging this pull request may close these issues.

5 participants