From 14a0ab6376f5f674d668bb6997ee0e905ccf2e76 Mon Sep 17 00:00:00 2001 From: Edmond Abraham <54119952+gitedmond@users.noreply.github.com> Date: Wed, 7 Oct 2026 15:55:07 -0400 Subject: [PATCH 1/4] GH-52021: [Ruby] Fix MemoryView data offsets for sliced arrays --- ruby/red-arrow/ext/arrow/memory-view.cpp | 7 ++-- ruby/red-arrow/test/test-memory-view.rb | 42 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/ruby/red-arrow/ext/arrow/memory-view.cpp b/ruby/red-arrow/ext/arrow/memory-view.cpp index 3869b45583d8..699b69f80f73 100644 --- a/ruby/red-arrow/ext/arrow/memory-view.cpp +++ b/ruby/red-arrow/ext/arrow/memory-view.cpp @@ -31,6 +31,7 @@ # undef private #endif +#include #include #include @@ -205,11 +206,13 @@ namespace red_arrow { private: void fill(const arrow::Array& array) { const auto array_data = array.data(); - const auto data = array_data->GetValuesSafe(1); - view_->data = const_cast(reinterpret_cast(data)); const auto type = std::static_pointer_cast(array.type()); view_->item_size = type->bit_width() / 8; + const auto byte_offset = + array_data->offset * std::max(view_->item_size, 1); + const auto data = array_data->GetValuesSafe(1, byte_offset); + view_->data = const_cast(reinterpret_cast(data)); view_->byte_size = view_->item_size * array.length(); } diff --git a/ruby/red-arrow/test/test-memory-view.rb b/ruby/red-arrow/test/test-memory-view.rb index 8aa5728a56a4..747b6b49beb7 100644 --- a/ruby/red-arrow/test/test-memory-view.rb +++ b/ruby/red-arrow/test/test-memory-view.rb @@ -29,6 +29,48 @@ def little_endian? [1].pack("s") == [1].pack("s<") end + [ + [Arrow::Int8Array, "c"], + [Arrow::UInt8Array, "C"], + [Arrow::Int16Array, "s"], + [Arrow::UInt16Array, "S"], + [Arrow::Int32Array, "l"], + [Arrow::UInt32Array, "L"], + [Arrow::Int64Array, "q"], + [Arrow::UInt64Array, "Q"], + [Arrow::FloatArray, "f"], + [Arrow::DoubleArray, "d"], + ].each do |array_class, format| + test("#{array_class.name}: sliced") do + values = (0...8).to_a + array = array_class.new(values) + item_size = [0].pack(format).bytesize + slices = [ + [array.slice(1, 3), values.slice(1, 3)], + [array.slice(4, 3), values.slice(4, 3)], + [array.slice(1, 6).slice(2, 3), values.slice(3, 3)], + [array.slice(4, 0), []], + [array.slice(8, 0), []], + ] + slices.each do |sliced_array, sliced_values| + Fiddle::MemoryView.export(sliced_array) do |memory_view| + assert_equal([ + format, + item_size, + item_size * sliced_values.size, + sliced_values.pack("#{format}*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + end + end + test("BooleanArray") do array = Arrow::BooleanArray.new([true] * 9) Fiddle::MemoryView.export(array) do |memory_view| From 0638ba33e3261341afeec7c6b55c8b1049f03a55 Mon Sep 17 00:00:00 2001 From: Edmond Abraham <54119952+gitedmond@users.noreply.github.com> Date: Thu, 8 Oct 2026 08:19:07 -0400 Subject: [PATCH 2/4] GH-52021: [Ruby] Address MemoryView review feedback --- ruby/red-arrow/ext/arrow/memory-view.cpp | 8 +- ruby/red-arrow/test/test-memory-view.rb | 369 ++++++++++++++--------- 2 files changed, 237 insertions(+), 140 deletions(-) diff --git a/ruby/red-arrow/ext/arrow/memory-view.cpp b/ruby/red-arrow/ext/arrow/memory-view.cpp index 699b69f80f73..cd14f591ee77 100644 --- a/ruby/red-arrow/ext/arrow/memory-view.cpp +++ b/ruby/red-arrow/ext/arrow/memory-view.cpp @@ -31,7 +31,6 @@ # undef private #endif -#include #include #include @@ -74,6 +73,9 @@ namespace red_arrow { } arrow::Status Visit(const arrow::BooleanArray& array) override { + if (array.offset() % 8 != 0) { + return arrow::Status::Invalid("Boolean array offset must be byte aligned"); + } fill(static_cast(array)); // Memory view doesn't support bit stream. We use one byte // for 8 elements. Users can't calculate the number of @@ -210,7 +212,7 @@ namespace red_arrow { std::static_pointer_cast(array.type()); view_->item_size = type->bit_width() / 8; const auto byte_offset = - array_data->offset * std::max(view_->item_size, 1); + array_data->offset * type->bit_width() / 8; const auto data = array_data->GetValuesSafe(1, byte_offset); view_->data = const_cast(reinterpret_cast(data)); view_->byte_size = view_->item_size * array.length(); @@ -232,6 +234,8 @@ namespace red_arrow { PrimitiveArrayGetter getter(view_); auto status = arrow_array->Accept(&getter); if (!status.ok()) { + delete static_cast(view_->private_data); + view_->private_data = nullptr; return false; } view_->readonly = true; diff --git a/ruby/red-arrow/test/test-memory-view.rb b/ruby/red-arrow/test/test-memory-view.rb index 747b6b49beb7..61a577c9a0d8 100644 --- a/ruby/red-arrow/test/test-memory-view.rb +++ b/ruby/red-arrow/test/test-memory-view.rb @@ -29,144 +29,215 @@ def little_endian? [1].pack("s") == [1].pack("s<") end - [ - [Arrow::Int8Array, "c"], - [Arrow::UInt8Array, "C"], - [Arrow::Int16Array, "s"], - [Arrow::UInt16Array, "S"], - [Arrow::Int32Array, "l"], - [Arrow::UInt32Array, "L"], - [Arrow::Int64Array, "q"], - [Arrow::UInt64Array, "Q"], - [Arrow::FloatArray, "f"], - [Arrow::DoubleArray, "d"], - ].each do |array_class, format| - test("#{array_class.name}: sliced") do - values = (0...8).to_a - array = array_class.new(values) - item_size = [0].pack(format).bytesize - slices = [ - [array.slice(1, 3), values.slice(1, 3)], - [array.slice(4, 3), values.slice(4, 3)], - [array.slice(1, 6).slice(2, 3), values.slice(3, 3)], - [array.slice(4, 0), []], - [array.slice(8, 0), []], - ] - slices.each do |sliced_array, sliced_values| - Fiddle::MemoryView.export(sliced_array) do |memory_view| - assert_equal([ - format, - item_size, - item_size * sliced_values.size, - sliced_values.pack("#{format}*"), - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) + sub_test_case("BooleanArray") do + test("offset: 0") do + array = Arrow::BooleanArray.new([true] * 9) + Fiddle::MemoryView.export(array) do |memory_view| + if little_endian? + template = "b" + else + template = "B" end + assert_equal([ + "#{template}8", + 1, + 2, + [(("1" * 9) + ("0" * 7))].pack("#{template}*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) end end - end - test("BooleanArray") do - array = Arrow::BooleanArray.new([true] * 9) - Fiddle::MemoryView.export(array) do |memory_view| - if little_endian? - template = "b" - else - template = "B" + test("offset: byte aligned") do + array = Arrow::BooleanArray.new(([false] * 8) + ([true] * 9)).slice(8, 9) + Fiddle::MemoryView.export(array) do |memory_view| + template = little_endian? ? "b" : "B" + assert_equal([ + "#{template}8", + 1, + 2, + [("1" * 9) + ("0" * 7)].pack("#{template}*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + + test("offset: not byte aligned") do + array = Arrow::BooleanArray.new([false, true, false]).slice(1, 2) + assert_raise(ArgumentError) do + Fiddle::MemoryView.export(array) do |memory_view| + end end - assert_equal([ - "#{template}8", - 1, - 2, - [(("1" * 9) + ("0" * 7))].pack("#{template}*"), - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) end end - test("Int8Array") do - values = [-(2 ** 7), 0, (2 ** 7) - 1] - array = Arrow::Int8Array.new(values) - Fiddle::MemoryView.export(array) do |memory_view| - assert_equal([ - "c", - 1, - values.size, - values.pack("c*"), - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) + sub_test_case("Int8Array") do + test("offset: 0") do + values = [-(2 ** 7), 0, (2 ** 7) - 1] + array = Arrow::Int8Array.new(values) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "c", + 1, + values.size, + values.pack("c*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + + test("offset: non-0") do + values = [0, 1, 2] + array = Arrow::Int8Array.new(values).slice(1, 2) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "c", + 1, + 1 * 2, + values.slice(1, 2).pack("c*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end end end - test("Int16Array") do - values = [-(2 ** 15), 0, (2 ** 15) - 1] - array = Arrow::Int16Array.new(values) - Fiddle::MemoryView.export(array) do |memory_view| - assert_equal([ - "s", - 2, - 2 * values.size, - values.pack("s*"), - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) + sub_test_case("Int16Array") do + test("offset: 0") do + values = [-(2 ** 15), 0, (2 ** 15) - 1] + array = Arrow::Int16Array.new(values) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "s", + 2, + 2 * values.size, + values.pack("s*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + + test("offset: non-0") do + values = [0, 1, 2] + array = Arrow::Int16Array.new(values).slice(1, 2) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "s", + 2, + 2 * 2, + values.slice(1, 2).pack("s*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end end end - test("Int32Array") do - values = [-(2 ** 31), 0, (2 ** 31) - 1] - array = Arrow::Int32Array.new(values) - Fiddle::MemoryView.export(array) do |memory_view| - assert_equal([ - "l", - 4, - 4 * values.size, - values.pack("l*"), - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) + sub_test_case("Int32Array") do + test("offset: 0") do + values = [-(2 ** 31), 0, (2 ** 31) - 1] + array = Arrow::Int32Array.new(values) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "l", + 4, + 4 * values.size, + values.pack("l*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + + test("offset: non-0") do + values = [0, 1, 2] + array = Arrow::Int32Array.new(values).slice(1, 2) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "l", + 4, + 4 * 2, + values.slice(1, 2).pack("l*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end end end - test("Int64Array") do - values = [-(2 ** 63), 0, (2 ** 63) - 1] - array = Arrow::Int64Array.new(values) - Fiddle::MemoryView.export(array) do |memory_view| - assert_equal([ - "q", - 8, - 8 * values.size, - values.pack("q*"), - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) + sub_test_case("Int64Array") do + test("offset: 0") do + values = [-(2 ** 63), 0, (2 ** 63) - 1] + array = Arrow::Int64Array.new(values) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "q", + 8, + 8 * values.size, + values.pack("q*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + + test("offset: non-0") do + values = [0, 1, 2] + array = Arrow::Int64Array.new(values).slice(1, 2) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "q", + 8, + 8 * 2, + values.slice(1, 2).pack("q*"), + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end end end @@ -284,23 +355,45 @@ def little_endian? end end - test("FixedSizeBinaryArray") do - values = ["\x01\x02", "\x03\x04", "\x05\x06"] - data_type = Arrow::FixedSizeBinaryDataType.new(2) - array = Arrow::FixedSizeBinaryArray.new(data_type, values) - Fiddle::MemoryView.export(array) do |memory_view| - assert_equal([ - "C2", - 2, - 2 * values.size, - values.join("").b, - ], - [ - memory_view.format, - memory_view.item_size, - memory_view.byte_size, - memory_view.to_s, - ]) + sub_test_case("FixedSizeBinaryArray") do + test("offset: 0") do + values = ["\x01\x02", "\x03\x04", "\x05\x06"] + data_type = Arrow::FixedSizeBinaryDataType.new(2) + array = Arrow::FixedSizeBinaryArray.new(data_type, values) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "C2", + 2, + 2 * values.size, + values.join("").b, + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end + end + + test("offset: non-0") do + values = ["\x01\x02", "\x03\x04", "\x05\x06"] + data_type = Arrow::FixedSizeBinaryDataType.new(2) + array = Arrow::FixedSizeBinaryArray.new(data_type, values).slice(1, 2) + Fiddle::MemoryView.export(array) do |memory_view| + assert_equal([ + "C2", + 2, + 2 * 2, + values.slice(1, 2).join("").b, + ], + [ + memory_view.format, + memory_view.item_size, + memory_view.byte_size, + memory_view.to_s, + ]) + end end end From 63fa4387ebc7e07d099ae78de87c14a0855c9d7f Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Fri, 9 Oct 2026 06:39:28 +0900 Subject: [PATCH 3/4] Implement available_p() to reject unsupported arrays --- ruby/red-arrow/ext/arrow/memory-view.cpp | 78 +++++++++++------------- ruby/red-arrow/test/test-memory-view.rb | 15 ++++- 2 files changed, 49 insertions(+), 44 deletions(-) diff --git a/ruby/red-arrow/ext/arrow/memory-view.cpp b/ruby/red-arrow/ext/arrow/memory-view.cpp index cd14f591ee77..27c8f1ffe1c3 100644 --- a/ruby/red-arrow/ext/arrow/memory-view.cpp +++ b/ruby/red-arrow/ext/arrow/memory-view.cpp @@ -68,6 +68,8 @@ namespace red_arrow { class PrimitiveArrayGetter : public arrow::ArrayVisitor { public: + // If view is nullptr, this only validates whether the visited + // array can be represented by memory view. explicit PrimitiveArrayGetter(memory_view *view) : view_(view) { } @@ -76,14 +78,16 @@ namespace red_arrow { if (array.offset() % 8 != 0) { return arrow::Status::Invalid("Boolean array offset must be byte aligned"); } - fill(static_cast(array)); + if (!view_) { + return arrow::Status::OK(); + } // Memory view doesn't support bit stream. We use one byte // for 8 elements. Users can't calculate the number of // elements from memory view but it's limitation of memory view. #ifdef ARROW_LITTLE_ENDIAN - view_->format = "b8"; + fill(static_cast(array), "b8"); #else - view_->format = "B8"; + fill(static_cast(array), "B8"); #endif view_->item_size = 1; view_->byte_size = (array.length() + 7) / 8; @@ -91,67 +95,59 @@ namespace red_arrow { } arrow::Status Visit(const arrow::Int8Array& array) override { - fill(static_cast(array)); - view_->format = "c"; + fill(static_cast(array), "c"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Int16Array& array) override { - fill(static_cast(array)); - view_->format = "s"; + fill(static_cast(array), "s"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Int32Array& array) override { - fill(static_cast(array)); - view_->format = "l"; + fill(static_cast(array), "l"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Int64Array& array) override { - fill(static_cast(array)); - view_->format = "q"; + fill(static_cast(array), "q"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::UInt8Array& array) override { - fill(static_cast(array)); - view_->format = "C"; + fill(static_cast(array), "C"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::UInt16Array& array) override { - fill(static_cast(array)); - view_->format = "S"; + fill(static_cast(array), "S"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::UInt32Array& array) override { - fill(static_cast(array)); - view_->format = "L"; + fill(static_cast(array), "L"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::UInt64Array& array) override { - fill(static_cast(array)); - view_->format = "Q"; + fill(static_cast(array), "Q"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::FloatArray& array) override { - fill(static_cast(array)); - view_->format = "f"; + fill(static_cast(array), "f"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::DoubleArray& array) override { - fill(static_cast(array)); - view_->format = "d"; + fill(static_cast(array), "d"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::FixedSizeBinaryArray& array) override { - fill(static_cast(array)); + if (!view_) { + return arrow::Status::OK(); + } auto priv = static_cast(view_->private_data); const auto type = std::static_pointer_cast( @@ -159,54 +155,51 @@ namespace red_arrow { std::ostringstream output; output << "C" << type->byte_width(); priv->format = output.str(); - view_->format = priv->format.c_str(); + fill(static_cast(array), priv->format.c_str()); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Date32Array& array) override { - fill(static_cast(array)); - view_->format = "l"; + fill(static_cast(array), "l"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Date64Array& array) override { - fill(static_cast(array)); - view_->format = "q"; + fill(static_cast(array), "q"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Time32Array& array) override { - fill(static_cast(array)); - view_->format = "l"; + fill(static_cast(array), "l"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Time64Array& array) override { - fill(static_cast(array)); - view_->format = "q"; + fill(static_cast(array), "q"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::TimestampArray& array) override { - fill(static_cast(array)); - view_->format = "q"; + fill(static_cast(array), "q"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Decimal128Array& array) override { - fill(static_cast(array)); - view_->format = "q2"; + fill(static_cast(array), "q2"); return arrow::Status::OK(); } arrow::Status Visit(const arrow::Decimal256Array& array) override { - fill(static_cast(array)); - view_->format = "q4"; + fill(static_cast(array), "q4"); return arrow::Status::OK(); } private: - void fill(const arrow::Array& array) { + void fill(const arrow::Array& array, const char *format) { + if (!view_) { + return; + } + view_->format = format; const auto array_data = array.data(); const auto type = std::static_pointer_cast(array.type()); @@ -250,7 +243,10 @@ namespace red_arrow { } bool primitive_array_available_p(VALUE obj) { - return true; + auto array = GARROW_ARRAY(RVAL2GOBJ(obj)); + auto arrow_array = garrow_array_get_raw(array); + PrimitiveArrayGetter validator(nullptr); + return arrow_array->Accept(&validator).ok(); } rb_memory_view_entry_t primitive_array_entry = { diff --git a/ruby/red-arrow/test/test-memory-view.rb b/ruby/red-arrow/test/test-memory-view.rb index 61a577c9a0d8..c86fd3bc4382 100644 --- a/ruby/red-arrow/test/test-memory-view.rb +++ b/ruby/red-arrow/test/test-memory-view.rb @@ -29,6 +29,16 @@ def little_endian? [1].pack("s") == [1].pack("s<") end + def memory_view_available?(target) + libruby = Fiddle.dlopen(nil) + rb_memory_view_available_p = Fiddle::Function.new( + libruby["rb_memory_view_available_p"], + [Fiddle::TYPE_UINTPTR_T], + Fiddle::TYPE_BOOL + ) + rb_memory_view_available_p.call(Fiddle.dlwrap(target)) + end + sub_test_case("BooleanArray") do test("offset: 0") do array = Arrow::BooleanArray.new([true] * 9) @@ -74,9 +84,8 @@ def little_endian? test("offset: not byte aligned") do array = Arrow::BooleanArray.new([false, true, false]).slice(1, 2) - assert_raise(ArgumentError) do - Fiddle::MemoryView.export(array) do |memory_view| - end + assert do + not memory_view_available?(array) end end end From 23902bdfffba9706e96fdcd23233cb8e0bf674b0 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Fri, 9 Oct 2026 06:40:13 +0900 Subject: [PATCH 4/4] Extract common code --- ruby/red-arrow/test/test-memory-view.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ruby/red-arrow/test/test-memory-view.rb b/ruby/red-arrow/test/test-memory-view.rb index c86fd3bc4382..408ff68d8f27 100644 --- a/ruby/red-arrow/test/test-memory-view.rb +++ b/ruby/red-arrow/test/test-memory-view.rb @@ -40,14 +40,17 @@ def memory_view_available?(target) end sub_test_case("BooleanArray") do + def template + if little_endian? + "b" + else + "B" + end + end + test("offset: 0") do array = Arrow::BooleanArray.new([true] * 9) Fiddle::MemoryView.export(array) do |memory_view| - if little_endian? - template = "b" - else - template = "B" - end assert_equal([ "#{template}8", 1, @@ -66,7 +69,6 @@ def memory_view_available?(target) test("offset: byte aligned") do array = Arrow::BooleanArray.new(([false] * 8) + ([true] * 9)).slice(8, 9) Fiddle::MemoryView.export(array) do |memory_view| - template = little_endian? ? "b" : "B" assert_equal([ "#{template}8", 1,