GH-37562: [Ruby] Add support for table.each_raw_record.to_a #37600

otegami · 2023-09-06T23:20:08Z

Rationale for this change

This change aligns the behavior of each_raw_record with standard Ruby practices by returning an enumerator when no block is provided

What changes are included in this PR?

Made Arrow::Table#each_raw_record and Arrow::RecordBatch#each_raw_record return Enumerator when it was called without block.
Added related tests
Resolved warnings related to duplicate test classes which were caused by GH-33749: [Ruby] Add Arrow::RecordBatch#each_raw_record #37137

Are these changes tested?

Yes.

Are there any user-facing changes?

No

Closes: [Ruby] Add support for table.each_raw_record.to_a #37562

…atch#each_raw_record without block

otegami · 2023-09-06T23:27:14Z

ruby/red-arrow/test/each-raw-record/test-struct-array.rb

In the following PR #37137, I missed the following warnings happened. So I fixed it in this PR.

red-arrow % bundle exec env DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:/tmp/local/lib $(rbenv which ruby) test/run-test.rb /Users/otegami/dev/project/arrow/ruby/red-arrow/test/raw-records/test-struct-array.rb:518: warning: method redefined; discarding old build /Users/otegami/dev/project/arrow/ruby/red-arrow/test/each-raw-record/test-struct-array.rb:658: warning: previous definition of build was here /Users/otegami/dev/project/arrow/ruby/red-arrow/test/raw-records/test-struct-array.rb:526: warning: method redefined; discarding old build /Users/otegami/dev/project/arrow/ruby/red-arrow/test/each-raw-record/test-struct-array.rb:666: warning: previous definition of build was here

otegami · 2023-09-06T23:27:18Z

ruby/red-arrow/ext/arrow/raw-records.cpp

@@ -306,6 +306,10 @@ namespace red_arrow {

  VALUE
  record_batch_each_raw_record(VALUE rb_record_batch){
+    if (!rb_block_given_p()) {
+      return rb_funcall(rb_record_batch, rb_intern("to_enum"), 1, ID2SYM(rb_intern("each_raw_record")));


If there is a good way to return a Enumerator object, could you teach me?🙏

We can use RETURN_SIZED_ENUMERATOR():

https://github.com/ruby/ruby/blob/05aaff2191cbe777d1efb915ab9652eeaa1c16b8/include/ruby/internal/intern/enumerator.h#L206-L209

fix: f59e5ff
Thanks. I tried to use RETURN_SIZED_ENUMERATOR()🙏

otegami · 2023-09-06T23:33:33Z

ruby/red-arrow/test/test-record-batch.rb

@@ -178,5 +178,29 @@ def setup
                     @record_batch[[:c, "a", -1, 3..4]])
      end
    end
+
+    sub_test_case("#each_raw_record") do


I was uncertain about the appropriate location to add the following test cases.
I added it here because each_raw_record is one of the methods for Arrow::Table or Arrow::RecordBatch and est/each-raw-record/* focuses on the test related to arrays.

ruby/red-arrow/test/each-raw-record/*

ruby/red-arrow/test/test-record-batch.rb

We can always use no block version like this:

diff --git a/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb b/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb index dbbbd79ee..05f0e90ef 100644 --- a/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb +++ b/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb @@ -22,12 +22,8 @@ module EachRawRecordBasicArraysTests [nil], [nil], ] - iterated_records = [] target = build({column: :null}, records) - target.each_raw_record do |record| - iterated_records << record - end - assert_equal(records, iterated_records) + assert_equal(records, target.each_raw_record.to_a) end def test_boolean

Because a returned Enumerator uses with-block version.

fix: bc23b9c

I misunderstood that we explicitly had to call the 'each_raw_record' with a block because I didn't understand how Enumerable#to_a worked.

I will unify tests with raw_records and each_raw_record at the following issue.

[Ruby] Unify tests with raw_records and each_raw_record #37563

kou · 2023-09-07T02:59:27Z

ruby/red-arrow/ext/arrow/raw-records.cpp

@@ -306,6 +306,10 @@ namespace red_arrow {

  VALUE
  record_batch_each_raw_record(VALUE rb_record_batch){
+    if (!rb_block_given_p()) {
+      return rb_funcall(rb_record_batch, rb_intern("to_enum"), 1, ID2SYM(rb_intern("each_raw_record")));


We can use RETURN_SIZED_ENUMERATOR():

https://github.com/ruby/ruby/blob/05aaff2191cbe777d1efb915ab9652eeaa1c16b8/include/ruby/internal/intern/enumerator.h#L206-L209

kou · 2023-09-07T02:59:32Z

ruby/red-arrow/ext/arrow/raw-records.cpp

@@ -306,6 +306,10 @@ namespace red_arrow {

  VALUE
  record_batch_each_raw_record(VALUE rb_record_batch){


Suggested change

record_batch_each_raw_record(VALUE rb_record_batch){

record_batch_each_raw_record(VALUE rb_record_batch) {

fix: 781f547

kou · 2023-09-07T03:02:33Z

ruby/red-arrow/test/test-record-batch.rb

@@ -178,5 +178,29 @@ def setup
                     @record_batch[[:c, "a", -1, 3..4]])
      end
    end
+
+    sub_test_case("#each_raw_record") do


We can always use no block version like this:

diff --git a/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb b/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb index dbbbd79ee..05f0e90ef 100644 --- a/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb +++ b/ruby/red-arrow/test/each-raw-record/test-basic-arrays.rb @@ -22,12 +22,8 @@ module EachRawRecordBasicArraysTests [nil], [nil], ] - iterated_records = [] target = build({column: :null}, records) - target.each_raw_record do |record| - iterated_records << record - end - assert_equal(records, iterated_records) + assert_equal(records, target.each_raw_record.to_a) end def test_boolean

Because a returned Enumerator uses with-block version.

This change makes the code more efficient and readable by directly returning an enumerator with a known size, rather than calling `to_enum` and `each_raw_record` methods via `rb_funcall`.

otegami · 2023-09-10T23:59:36Z

@kou
Thank you for reviewing 🙏 I've fixed all the points you commented on.

kou · 2023-09-11T00:57:32Z

ruby/red-arrow/ext/arrow/raw-records.cpp

    auto garrow_record_batch = GARROW_RECORD_BATCH(RVAL2GOBJ(rb_record_batch));
    auto record_batch = garrow_record_batch_get_raw(garrow_record_batch).get();
+    RETURN_SIZED_ENUMERATOR(rb_record_batch, 0, 0, record_batch->num_rows());


We should use nullptr for a null pointer in C++:

Suggested change

RETURN_SIZED_ENUMERATOR(rb_record_batch, 0, 0, record_batch->num_rows());

RETURN_SIZED_ENUMERATOR(rb_record_batch, 0, nullptr, record_batch->num_rows());

Thank you so much!
fix: 3563046

* @param[in] argc Number of objects of `argv`. * @param[in] argv Arguments passed to the current method.

We can pass the 0 to argc because it is literally the number of objects of argv.
On the other hand, we cannot pass the 0. to argv because we want to say there is no passed method pointer.
https://github.com/ruby/ruby/blob/05aaff2191cbe777d1efb915ab9652eeaa1c16b8/include/ruby/internal/intern/enumerator.h#L201C4-L202

ruby/red-arrow/ext/arrow/raw-records.cpp

…assed to the current method

kou

+1

conbench-apache-arrow · 2023-09-11T08:15:49Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit e56726c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ache#37600) ### Rationale for this change This change aligns the behavior of `each_raw_record` with standard Ruby practices by returning an enumerator when no block is provided ### What changes are included in this PR? - Made `Arrow::Table#each_raw_record` and `Arrow::RecordBatch#each_raw_record` return Enumerator when it was called without block. - Added related tests - Resolved warnings related to duplicate test classes which were caused by apache#37137 ### Are these changes tested? Yes. ### Are there any user-facing changes? No * Closes: apache#37562 Authored-by: otegami <a.s.takuya1026@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

otegami added 2 commits September 7, 2023 06:53

Fix LocalJumpError in Arrow::Table#each_raw_record and Arrow::RecordB…

291044e

…atch#each_raw_record without block

Resolve warnings related to duplicate test classes

340cb64

otegami requested a review from kou as a code owner September 6, 2023 23:20

github-actions bot added Component: Ruby awaiting review Awaiting review labels Sep 6, 2023

otegami commented Sep 6, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 6, 2023

kou reviewed Sep 7, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 7, 2023

otegami added 4 commits September 8, 2023 22:39

Use RETURN_SIZED_ENUMERATOR instead of rb_funcall

f59e5ff

This change makes the code more efficient and readable by directly returning an enumerator with a known size, rather than calling `to_enum` and `each_raw_record` methods via `rb_funcall`.

Fix code format

781f547

Enumerator uses with-block version.

bc23b9c

Remove redundant test cases that are already covered by array tests

71b3ae8

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 10, 2023

otegami requested a review from kou September 10, 2023 23:59

kou reviewed Sep 11, 2023

View reviewed changes

github-actions bot added awaiting review Awaiting review awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 11, 2023

Use nullptr for a null pointer in C++ because there is no arguments p…

3563046

…assed to the current method

github-actions bot added awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Sep 11, 2023

otegami requested a review from kou September 11, 2023 04:27

kou approved these changes Sep 11, 2023

View reviewed changes

kou merged commit e56726c into apache:main Sep 11, 2023
12 checks passed

kou removed the awaiting committer review Awaiting committer review label Sep 11, 2023

github-actions bot added the awaiting merge Awaiting merge label Sep 11, 2023

otegami deleted the ruby-support-each-raw-record-no-block-given branch September 11, 2023 05:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37562: [Ruby] Add support for table.each_raw_record.to_a #37600

GH-37562: [Ruby] Add support for table.each_raw_record.to_a #37600

otegami commented Sep 6, 2023 •

edited by kou

Loading

otegami Sep 6, 2023

otegami Sep 6, 2023

kou Sep 7, 2023

otegami Sep 10, 2023

otegami Sep 6, 2023 •

edited

Loading

kou Sep 7, 2023

otegami Sep 10, 2023

kou Sep 7, 2023

kou Sep 7, 2023

otegami Sep 10, 2023

kou Sep 7, 2023

otegami commented Sep 10, 2023

kou Sep 11, 2023

otegami Sep 11, 2023 •

edited

Loading

kou left a comment

conbench-apache-arrow bot commented Sep 11, 2023

		@@ -306,6 +306,10 @@ namespace red_arrow {

		VALUE
		record_batch_each_raw_record(VALUE rb_record_batch){

	RETURN_SIZED_ENUMERATOR(rb_record_batch, 0, 0, record_batch->num_rows());
	RETURN_SIZED_ENUMERATOR(rb_record_batch, 0, nullptr, record_batch->num_rows());

GH-37562: [Ruby] Add support for table.each_raw_record.to_a #37600

GH-37562: [Ruby] Add support for table.each_raw_record.to_a #37600

Conversation

otegami commented Sep 6, 2023 • edited by kou Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

otegami Sep 6, 2023 • edited Loading

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

otegami commented Sep 10, 2023

Choose a reason for hiding this comment

otegami Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Sep 11, 2023

otegami commented Sep 6, 2023 •

edited by kou

Loading

otegami Sep 6, 2023 •

edited

Loading

otegami Sep 11, 2023 •

edited

Loading