ARROW-5155: [GLib][Ruby] Add support for building union arrays from data type #4127

mrkn · 2019-04-09T05:42:35Z

This is separated from #3723.
This should be merged after #3723.

mrkn · 2019-04-10T01:43:33Z

@kou This is ready to review.
Could you please have a look this again?

kou · 2019-04-10T22:08:27Z

c_glib/arrow-glib/composite-array.h

+                              gchar **field_names,
+                              gsize n_field_names,
+                              guint8 *type_codes,
+                              gsize n_type_codes,


I don't want to provide API that requires many arguments. Because the API isn't easy to use.

How about using GArrowSparseUnionDataType?

diff --git a/c_glib/arrow-glib/composite-array.cpp b/c_glib/arrow-glib/composite-array.cpp index b202fb45..aac3c24c 100644 --- a/c_glib/arrow-glib/composite-array.cpp +++ b/c_glib/arrow-glib/composite-array.cpp @@ -366,6 +366,54 @@ garrow_sparse_union_array_new(GArrowInt8Array *type_ids, } } +/** + * garrow_sparse_union_array_new_data_type: + * @data_type: The data type for the sparse array. + * @type_ids: The field type IDs for each value as #GArrowInt8Array. + * @fields: (element-type GArrowArray): The arrays for each field + * as #GList of #GArrowArray. + * @error: (nullable): Return location for a #GError or %NULL. + * + * Returns: (nullable): A newly created #GArrowSparseUnionArray + * or %NULL on error. + * + * Since: 0.14.0 + */ +GArrowSparseUnionArray * +garrow_sparse_union_array_new_data_type(GArrowSparseUnionDataType *data_type, + GArrowInt8Array *type_ids, + GList *fields, + GError **error) +{ + auto arrow_data_type = garrow_data_type_get_raw(GARROW_DATA_TYPE(data_type)); + auto arrow_union_data_type = + std::static_pointer_cast<arrow::UnionType>(arrow_data_type); + std::vector<std::string> arrow_field_names; + for (const auto &arrow_field : arrow_union_data_type->children()) { + arrow_field_names.push_back(arrow_field->name()); + } + std::vector<uint8_t> arrow_type_codes(arrow_union_data_type->type_codes()); + auto arrow_type_ids = garrow_array_get_raw(GARROW_ARRAY(type_ids)); + std::vector<std::shared_ptr<arrow::Array>> arrow_fields; + for (auto node = fields; node; node = node->next) { + auto *field = GARROW_ARRAY(node->data); + arrow_fields.push_back(garrow_array_get_raw(field)); + } + std::shared_ptr<arrow::Array> arrow_union_array; + auto status = arrow::UnionArray::MakeSparse(*arrow_type_ids, + arrow_fields, + arrow_field_names, + arrow_type_codes, + &arrow_union_array); + if (garrow_error_check(error, + status, + "[sparse-union-array][new][data-type]")) { + return GARROW_SPARSE_UNION_ARRAY(garrow_array_new_raw(&arrow_union_array)); + } else { + return NULL; + } +} + G_DEFINE_TYPE(GArrowDenseUnionArray, garrow_dense_union_array, diff --git a/c_glib/arrow-glib/composite-array.h b/c_glib/arrow-glib/composite-array.h index a181ffcc..c65c1d66 100644 --- a/c_glib/arrow-glib/composite-array.h +++ b/c_glib/arrow-glib/composite-array.h @@ -108,6 +108,11 @@ GArrowSparseUnionArray * garrow_sparse_union_array_new(GArrowInt8Array *type_ids, GList *fields, GError **error); +GArrowSparseUnionArray * +garrow_sparse_union_array_new_data_type(GArrowSparseUnionDataType *data_type, + GArrowInt8Array *type_ids, + GList *fields, + GError **error); #define GARROW_TYPE_DENSE_UNION_ARRAY (garrow_dense_union_array_get_type()) diff --git a/c_glib/test/test-sparse-union-array.rb b/c_glib/test/test-sparse-union-array.rb index 721f95c1..4a9e7c81 100644 --- a/c_glib/test/test-sparse-union-array.rb +++ b/c_glib/test/test-sparse-union-array.rb @@ -18,32 +18,69 @@ class TestSparseUnionArray < Test::Unit::TestCase include Helper::Buildable - def setup - type_ids = build_int8_array([0, 1, nil, 1, 0]) - fields = [ - build_int16_array([1, nil, nil, nil, 5]), - build_string_array([nil, "b", nil, "d", nil]), - ] - @array = Arrow::SparseUnionArray.new(type_ids, fields) - end + sub_test_case(".new") do + sub_test_case("default") do + def setup + type_ids = build_int8_array([0, 1, nil, 1, 0]) + fields = [ + build_int16_array([1, nil, nil, nil, 5]), + build_string_array([nil, "b", nil, "d", nil]), + ] + @array = Arrow::SparseUnionArray.new(type_ids, fields) + end - def test_value_data_type - fields = [ - Arrow::Field.new("0", Arrow::Int16DataType.new), - Arrow::Field.new("1", Arrow::StringDataType.new), - ] - assert_equal(Arrow::SparseUnionDataType.new(fields, [0, 1]), - @array.value_data_type) - end + def test_value_data_type + fields = [ + Arrow::Field.new("0", Arrow::Int16DataType.new), + Arrow::Field.new("1", Arrow::StringDataType.new), + ] + assert_equal(Arrow::SparseUnionDataType.new(fields, [0, 1]), + @array.value_data_type) + end - def test_field - assert_equal([ - build_int16_array([1, nil, nil, nil, 5]), - build_string_array([nil, "b", nil, "d", nil]), - ], - [ - @array.get_field(0), - @array.get_field(1), - ]) + def test_field + assert_equal([ + build_int16_array([1, nil, nil, nil, 5]), + build_string_array([nil, "b", nil, "d", nil]), + ], + [ + @array.get_field(0), + @array.get_field(1), + ]) + end + end + + sub_test_case("DataType") do + def setup + data_type_fields = [ + Arrow::Field.new("number", Arrow::Int16DataType.new), + Arrow::Field.new("text", Arrow::StringDataType.new), + ] + type_codes = [11, 13] + @data_type = Arrow::SparseUnionDataType.new(data_type_fields, type_codes) + type_ids = build_int8_array([0, 1, nil, 1, 0]) + fields = [ + build_int16_array([1, nil, nil, nil, 5]), + build_string_array([nil, "b", nil, "d", nil]), + ] + @array = Arrow::SparseUnionArray.new(@data_type, type_ids, fields) + end + + def test_value_data_type + assert_equal(@data_type, + @array.value_data_type) + end + + def test_field + assert_equal([ + build_int16_array([1, nil, nil, nil, 5]), + build_string_array([nil, "b", nil, "d", nil]), + ], + [ + @array.get_field(0), + @array.get_field(1), + ]) + end + end end end

OK. I'll rewrite the code to do so.

mrkn · 2019-04-24T01:27:57Z

@kou It's ready to review again. Please have a look.

codecov-io · 2019-04-24T02:08:46Z

Codecov Report

Merging #4127 into master will increase coverage by 1.4%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4127      +/-   ##
==========================================
+ Coverage   87.77%   89.18%    +1.4%     
==========================================
  Files         758      617     -141     
  Lines       92506    82202   -10304     
  Branches     1251        0    -1251     
==========================================
- Hits        81201    73315    -7886     
+ Misses      11188     8887    -2301     
+ Partials      117        0     -117

Impacted Files	Coverage Δ
python/pyarrow/_plasma.pyx	`91.27% <0%> (-0.06%)`	⬇️
python/pyarrow/pandas_compat.py	`96.7% <0%> (-0.01%)`	⬇️
python/pyarrow/_csv.pyx	`99.15% <0%> (-0.01%)`	⬇️
cpp/src/arrow/array-test.cc	`100% <0%> (ø)`	⬆️
python/pyarrow/tests/test_array.py	`96.79% <0%> (ø)`	⬆️
cpp/src/arrow/array-binary-test.cc	`100% <0%> (ø)`	⬆️
python/pyarrow/types.pxi	`69.27% <0%> (ø)`	⬆️
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/ipc/file_reader.go
... and 143 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 532450d...e625556. Read the comment docs.

kou

Could you check my comments?

kou · 2019-04-24T02:38:38Z

c_glib/arrow-glib/composite-array.cpp

+  for (const auto &arrow_field : arrow_union_data_type->children()) {
+    arrow_field_names.push_back(arrow_field->name());
+  }
+  std::vector<uint8_t> arrow_type_codes(arrow_union_data_type->type_codes());


Can we use arrow_union_data_type->type_codes() directly for arrow::UnionArray::MakeSparse() instead of copying it?

kou · 2019-04-24T02:38:57Z

c_glib/arrow-glib/composite-array.cpp

@@ -420,6 +468,57 @@ garrow_dense_union_array_new(GArrowInt8Array *type_ids,
  }
 }

+/**
+ * garrow_dense_union_array_new_data_type:
+ * @data_type: The data type for the sparse array.


"sparse" -> "dense"

kou · 2019-04-24T02:45:02Z

c_glib/test/test-dense-union-array.rb

-    @array = Arrow::DenseUnionArray.new(type_ids, value_offsets, fields)
-  end
+  sub_test_case(".new") do
+    def setup


Could you put more sub test cases under ".new" sub test case?

sub_test_case(".new") do sub_test_case("default") do # or "no DataType"? end sub_test_case("DataType") do end end

kou · 2019-04-24T03:10:59Z

c_glib/test/test-dense-union-array.rb

+      ]
+      type_codes = [11, 13]
+      @data_type = Arrow::DenseUnionDataType.new(data_type_fields, type_codes)
+      type_ids = build_int8_array([0, 1, nil, 1, 0])


Is this right?
[11, 13, nil, 13, 13]?

Yes, type ids must be [11, 13, nil, 13, 11].
But, it doesn't affect the tests so I couldn't notice the mistake.

We should add element accessor to union arrays and test union array values later.

I understand it.

kou · 2019-04-24T03:11:53Z

c_glib/test/test-sparse-union-array.rb

+      ]
+      type_codes = [11, 13]
+      @data_type = Arrow::SparseUnionDataType.new(data_type_fields, type_codes)
+      type_ids = build_int8_array([0, 1, nil, 1, 0])


Is this right?
[11, 13, nil, 13, 11]?

mrkn · 2019-04-25T01:34:16Z

@kou I've done to fix code for your comments.

kou

Thanks.
I'll push a fix and merge this.

kou · 2019-04-25T02:24:41Z

c_glib/test/test-dense-union-array.rb

+        ]
+        type_codes = [11, 13]
+        @data_type = Arrow::DenseUnionDataType.new(data_type_fields, type_codes)
+        type_ids = build_int8_array([11, 13, nil, 11, 13])


[11, 13, nil, 11, 13] should be [11, 13, nil, 13, 13] because number field only has one element and text field has three elements.

mrkn mentioned this pull request Apr 9, 2019

ARROW-4622: [C++][Python] MakeDense and MakeSparse in UnionArray should accept a vector of Field #3723

Closed

9 tasks

mrkn force-pushed the glib_ruby_make_union_array_with_field_names branch from 6421d3b to ec67279 Compare April 10, 2019 01:35

mrkn changed the title ~~WIP: [GLib][Ruby]MakeDense and MakeSparse in UnionArray should accept a vector of Field~~ ARROW-5155: [GLib][Ruby]MakeDense and MakeSparse in UnionArray should accept a vector of Field Apr 10, 2019

kou reviewed Apr 10, 2019

View reviewed changes

mrkn force-pushed the glib_ruby_make_union_array_with_field_names branch from ec67279 to d755ba7 Compare April 22, 2019 09:30

mrkn added 2 commits April 23, 2019 17:59

[GLib] Add garrow_sparse_union_array_new_data_type

c8793d5

[GLib] Add garrow_dense_union_array_new_data_type

5ad5572

mrkn force-pushed the glib_ruby_make_union_array_with_field_names branch 2 times, most recently from 8406d4b to 5ad5572 Compare April 24, 2019 01:21

[Ruby] Use new constructors of union arrays

606a04c

kou requested changes Apr 24, 2019

View reviewed changes

mrkn added 3 commits April 25, 2019 10:25

[GLib] Stop copying a type_code vector

f1bfa07

[GLib] Fix comment

d550dc9

[GLib] Fix test cases

f82ac3d

kou approved these changes Apr 25, 2019

View reviewed changes

Fix test data

e625556

kou changed the title ~~ARROW-5155: [GLib][Ruby]MakeDense and MakeSparse in UnionArray should accept a vector of Field~~ ARROW-5155: [GLib][Ruby] MakeDense and MakeSparse in UnionArray should accept a vector of Field Apr 25, 2019

kou changed the title ~~ARROW-5155: [GLib][Ruby] MakeDense and MakeSparse in UnionArray should accept a vector of Field~~ ARROW-5155: [GLib][Ruby] Add support for building union arrays from data type Apr 25, 2019

kou closed this in ecfb807 Apr 25, 2019

mrkn deleted the glib_ruby_make_union_array_with_field_names branch April 25, 2019 06:39

asfimport mentioned this pull request Apr 25, 2019

[GLib][Ruby] Add support for building union arrays from data type #16710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-5155: [GLib][Ruby] Add support for building union arrays from data type #4127

ARROW-5155: [GLib][Ruby] Add support for building union arrays from data type #4127

mrkn commented Apr 9, 2019

mrkn commented Apr 10, 2019

kou Apr 10, 2019

mrkn Apr 22, 2019

mrkn commented Apr 24, 2019

codecov-io commented Apr 24, 2019 •

edited

kou left a comment

kou Apr 24, 2019

kou Apr 24, 2019

kou Apr 24, 2019

kou Apr 24, 2019

mrkn Apr 25, 2019

kou Apr 25, 2019

mrkn Apr 25, 2019

kou Apr 24, 2019

mrkn commented Apr 25, 2019

kou left a comment

kou Apr 25, 2019

mrkn Apr 25, 2019

ARROW-5155: [GLib][Ruby] Add support for building union arrays from data type #4127

ARROW-5155: [GLib][Ruby] Add support for building union arrays from data type #4127

Conversation

mrkn commented Apr 9, 2019

mrkn commented Apr 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkn commented Apr 24, 2019

codecov-io commented Apr 24, 2019 • edited

Codecov Report

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkn commented Apr 25, 2019

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 24, 2019 •

edited