Skip to content

Conversation

@kou
Copy link
Member

@kou kou commented Apr 6, 2018

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

Missing word: … It's more efficient …

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!
I've fixed it.

Copy link
Member

Choose a reason for hiding this comment

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

Converting to std::string and inserting then into the Builder is still not the most efficient way, it would be better if we could directly pass the values to the StringBuilder instance. This has not be in this PR but I made a JIRA about that: https://issues.apache.org/jira/browse/ARROW-2411

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense.
I'll create a pull request.

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@xhochy
Copy link
Member

xhochy commented Apr 10, 2018

@kou feel free to merge or rebase on the newly added Append method.

@kou kou force-pushed the glib-string-array-builder-append branch from e7c35f3 to 5bc6e8d Compare April 11, 2018 00:15
@kou
Copy link
Member Author

kou commented Apr 11, 2018

OK.
I've rebased and changed to use the new Append() method.

If CI is passed, I'll merge this.

@kou kou closed this in 1ee7d11 Apr 11, 2018
@kou kou deleted the glib-string-array-builder-append branch April 11, 2018 03:35
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.

2 participants