Skip to content

Conversation

@mrkn
Copy link
Member

@mrkn mrkn commented Feb 12, 2019

MakeBuilder doesn't preserve the field name in the given ListType.
I think this is a bug.

@mrkn mrkn changed the title [C++] Fix MakeBuilder to preserve ListType's field name ARROW-4535: [C++] Fix MakeBuilder to preserve ListType's field name Feb 12, 2019
Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1, yes I agree with you

Copy link
Member

Choose a reason for hiding this comment

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

I think that we don't need this assertion.
This is tests on C++ layer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted.

@mrkn mrkn force-pushed the fix_make_builder_to_preserve_field_name_of_list_type branch from d5b0e68 to 34f9fd7 Compare February 12, 2019 04:08
@kou
Copy link
Member

kou commented Feb 12, 2019

The test failure on Travis CI is related to a garrow_list_array_new() bug.
It must be passed GArrowFiled * instead of using arrow::list() internally.

We need to fix this bug. The fix of this breaks backward compatibility but this is acceptable. Because normal users don't use garrow_list_array_new() directly.

@mrkn
Copy link
Member Author

mrkn commented Feb 12, 2019

@kou I'll make another pull-request to modify garrow_list_array_new().

@kou
Copy link
Member

kou commented Feb 12, 2019

Thanks!

MakeBuilder doesn't preserve the field name in the given ListType.
I think this is a bug.
@mrkn mrkn force-pushed the fix_make_builder_to_preserve_field_name_of_list_type branch from 34f9fd7 to 54c18c7 Compare February 12, 2019 06:54
@mrkn mrkn force-pushed the fix_make_builder_to_preserve_field_name_of_list_type branch from 54c18c7 to e4d8597 Compare February 12, 2019 08:36
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 xhochy closed this in 35b2871 Feb 12, 2019
@mrkn mrkn deleted the fix_make_builder_to_preserve_field_name_of_list_type branch February 12, 2019 14:42
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.

4 participants