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-2604: [Java] Add convenience method to VarCharVector to set Text #2071

Conversation

BryanCutler
Copy link
Member

This adds a convenience method to easily set values in VarCharVector using the friendly type Text. This allows the user to set values without having to think about the correct encoding. Text objects can be constructed from a String using the constructor Text(String string).

Extended existing test to set/get a VarCharVector using Text objects.

@BryanCutler
Copy link
Member Author

BryanCutler commented May 21, 2018

I had also wondered why there was no method to set a String directly in VarCharVector but after looking into it, it seems like the "friendly type" is Text - this is the type of object that the vector returns in getObject. It seems like maybe the Text class is being encouraged over String, so I only added the ability to set a Text and not String directly, although it is easy to create Text from String. I'm neutral on whether or not to also add set methods for a String, but maybe it would make the API too crowded and be confusing to the user. What are your thoughts @siddharthteotia @icexelloss @xhochy ?

btw, it seems hadoop users are familiar with using Text over String..

Copy link
Contributor

@icexelloss icexelloss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@icexelloss
Copy link
Contributor

I am curious about the choice of using Text over String too, maybe @jacques-n can shed some lights?

@BryanCutler
Copy link
Member Author

BryanCutler commented May 23, 2018

I'll commit this later today if no more comments. We can discuss further if there is still interest in adding an api directly for setting Strings.

Copy link
Contributor

@siddharthteotia siddharthteotia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@BryanCutler
Copy link
Member Author

Thanks @icexelloss and @siddharthteotia ! merged to master

@BryanCutler BryanCutler deleted the java-varchar-from-string-ARROW-2604 branch May 23, 2018 22:30
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.

3 participants