kou commented on code in PR #52286:
URL: https://github.com/apache/arrow/pull/52286#discussion_r4216528838
##########
ruby/red-arrow/ext/arrow/memory-view.cpp:
##########
@@ -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<uint8_t>(1);
- view_->data = const_cast<void *>(reinterpret_cast<const void *>(data));
const auto type =
std::static_pointer_cast<const arrow::FixedWidthType>(array.type());
view_->item_size = type->bit_width() / 8;
+ const auto byte_offset =
+ array_data->offset * std::max<int64_t>(view_->item_size, 1);
Review Comment:
Is this `std::max<int64_t>()` for non byte aligned (`offset % 8 != 0`)
boolean array? We should reject the case because we can't represent the case by
MemoryView.
##########
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), []],
Review Comment:
Do we need all cases? We want to remove needless cases as much as possible
to focus on only important parts.
##########
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
Review Comment:
We don't want to define tests dynamically. Could you follow existing style?
We can use the following style here:
```ruby
sub_test_case("Int8Array") do
test("offset: 0") do
# Existing test("Int8Array") content
end
test("offset: non-0") do
# This PR case
end
end
sub_test_case("Int8Array") do
# ...
end
# ...
```
##########
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
Review Comment:
We don't want to use `each {assert_equal(...)}` for easy to debug. If we use
the style, we can't run only the target case. We need to run all cases. And all
cases aren't executed if one case is failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
