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-7742: [GLib] Add support for MapArray #6436

Closed
wants to merge 10 commits into from

Conversation

shiro615
Copy link
Contributor

This PR add support for GArrowMapArrayBuilder, GArrowMapArray, and GArrowMapDataType.

@github-actions
Copy link

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Could you check my comments?

{
GObjectClass *gobject_class;

gobject_class = G_OBJECT_CLASS(klass);
Copy link
Member

Choose a reason for hiding this comment

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

Could you use auto gobject_class = ... here?

* @data_type: #GArrowMapDataType for the map.
* @error: (nullable): Return location for a #GError or %NULL.
*
* Returns: A newly created #GArrowMapArrayBuilder.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add on success, %NULL on error?

auto builder = garrow_array_builder_new(arrow_data_type,
error,
"[map-array-builder][new]");
return GARROW_MAP_ARRAY_BUILDER(builder);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add NULL check?

garrow_array_builder_get_raw(GARROW_ARRAY_BUILDER(builder)));

auto status = arrow_builder->Append();
return garrow_error_check(error,
Copy link
Member

Choose a reason for hiding this comment

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

Could you use garrow::check()?

arrow_items,
arrow_memory_pool,
&arrow_array);
if (garrow_error_check(error, status, "[map-array][new]")) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you use garrow::check()?

auto data_type = GARROW_DATA_TYPE(map_data_type);
auto arrow_data_type = garrow_data_type_get_raw(data_type);
auto arrow_map_data_type =
static_cast<arrow::MapType *>(arrow_data_type.get());
Copy link
Member

Choose a reason for hiding this comment

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

Can we use std::static_pointer_cast<arrow::MapType>(arrow_data_type) here?

auto data_type = GARROW_DATA_TYPE(map_data_type);
auto arrow_data_type = garrow_data_type_get_raw(data_type);
auto arrow_map_data_type =
static_cast<arrow::MapType *>(arrow_data_type.get());
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.


def setup
offsets = build_int32_array([0, 2, 5])
@keys = build_string_array(["a", "b", "c", "d", "e"])
Copy link
Member

Choose a reason for hiding this comment

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

@keys -> keys.

def setup
offsets = build_int32_array([0, 2, 5])
@keys = build_string_array(["a", "b", "c", "d", "e"])
@items = build_int16_array([0, 1, 2, 3, 4])
Copy link
Member

Choose a reason for hiding this comment

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

@items -> items.

* @builder: A #GArrowMapArrayBuilder.
*
* Returns: (transfer none): The #GArrowArrayBuilder for struct values.
* This is used instead of garrow_map_array_builder_get_key_builder() and garrow_map_array_builder_get_item_builder().
Copy link
Member

Choose a reason for hiding this comment

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

How about this?

  * Returns: (transfer none): The #GArrowArrayBuilder to add map entries as struct values.
  *   This can be used instead of garrow_map_array_builder_get_key_builder() and
  *   garrow_map_array_builder_get_item_builder(). You can build map entries as a list of
  *   struct values with this builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for improving document. I've applied this.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou closed this in 12aa05a Feb 20, 2020
@shiro615 shiro615 deleted the glib-map-array branch February 21, 2020 12:49
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.

None yet

2 participants