Copilot commented on code in PR #50386:
URL: https://github.com/apache/arrow/pull/50386#discussion_r3527464984
##########
ruby/red-arrow-format/lib/arrow-format/bitmap-builder.rb:
##########
@@ -0,0 +1,87 @@
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
Review Comment:
The ASF license header is missing the standard first line ("Licensed to the
Apache Software Foundation (ASF) under one"). This makes the header
inconsistent with most Ruby files in this repo and can trip automated
license/header checks.
##########
ruby/red-arrow-format/lib/arrow-format/bitmap-builder.rb:
##########
@@ -0,0 +1,87 @@
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+require_relative "buffer-alignable"
+
+module ArrowFormat
+ class DenseBitmapBuilder
+ include BufferAlignable
+
+ def initialize
+ @buffer = +"".b
+ @n_bits = 0
+ @byte = 0
+ end
+
+ def append(value)
+ @byte |= 1 << @n_bits if value
+ @n_bits += 1
+ flush if @n_bits == 8
+ self
+ end
+
+ def finish
+ flush if @n_bits > 0
+ padding_size = buffer_padding_size(@buffer)
+ @buffer.append_as_bytes(padding(padding_size)) if padding_size > 0
+ @buffer.freeze
+ IO::Buffer.for(@buffer)
+ end
+
+ private
+ def flush
+ @buffer.append_as_bytes([@byte].pack("C"))
+ @n_bits = 0
+ @byte = 0
+ end
Review Comment:
`String#append_as_bytes` is likely not available on supported Ruby versions;
`flush` should append the packed byte using standard `String#<<` to avoid a
runtime `NoMethodError`.
##########
ruby/red-arrow-format/test/test-dense-bitmap-builder.rb:
##########
@@ -0,0 +1,45 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+class TestDenseBitmapBuilder < Test::Unit::TestCase
Review Comment:
This test references `ArrowFormat::DenseBitmapBuilder`, but `test/helper.rb`
only requires `arrow-format` (which doesn't load
`arrow-format/bitmap-builder`). Add an explicit require so the test doesn't
depend on incidental load order.
##########
ruby/red-arrow-format/lib/arrow-format/bitmap-builder.rb:
##########
@@ -0,0 +1,87 @@
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+require_relative "buffer-alignable"
+
+module ArrowFormat
+ class DenseBitmapBuilder
+ include BufferAlignable
+
+ def initialize
+ @buffer = +"".b
+ @n_bits = 0
+ @byte = 0
+ end
+
+ def append(value)
+ @byte |= 1 << @n_bits if value
+ @n_bits += 1
+ flush if @n_bits == 8
+ self
+ end
+
+ def finish
+ flush if @n_bits > 0
+ padding_size = buffer_padding_size(@buffer)
+ @buffer.append_as_bytes(padding(padding_size)) if padding_size > 0
+ @buffer.freeze
+ IO::Buffer.for(@buffer)
+ end
+
+ private
+ def flush
+ @buffer.append_as_bytes([@byte].pack("C"))
+ @n_bits = 0
+ @byte = 0
+ end
+ end
+
+ class SparseBitmapBuilder
+ include BufferAlignable
+
+ def initialize(default)
+ @default = default
+ @indexes = []
+ end
+
+ def set(index)
+ @indexes << index
+ end
+
+ def finish(size)
+ builder = DenseBitmapBuilder.new
+ not_set_value = @default
+ set_value = !@default
+ previous_index = 0
+ @indexes.sort.each do |index|
+ previous_index.upto(index - 1) do
+ builder.append(not_set_value)
+ end
+ builder.append(set_value)
+ previous_index = index + 1
+ end
+ if previous_index.zero?
+ n_remains = size
+ else
+ n_remains = size - previous_index
+ end
+ n_remains.times do
+ builder.append(not_set_value)
+ end
+ builder.finish
+ end
Review Comment:
`SparseBitmapBuilder#finish` can silently generate an incorrect bitmap when
`set` is called with duplicate indices (it will emit extra bits) or with
indices outside `[0, size)` (it can emit more than `size` bits). De-duplicate
and validate indices/size so output length is always exactly `size` bits.
##########
ruby/red-arrow-format/test/test-sparse-bitmap-builder.rb:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+class TestSparseBitmapBuilder < Test::Unit::TestCase
Review Comment:
This test references `ArrowFormat::SparseBitmapBuilder`, but
`test/helper.rb` only requires `arrow-format` (which doesn't load
`arrow-format/bitmap-builder`). Add an explicit require so the test doesn't
depend on incidental load order.
##########
ruby/red-arrow-format/lib/arrow-format/integration/json-reader.rb:
##########
@@ -205,18 +205,11 @@ def read_schema
end
def read_bitmap(bitmap)
- buffer = +"".b
- bitmap.each_slice(8) do |bits|
- byte = 0
- while bits.size < 8
- bits << 0
- end
- bits.reverse_each do |bit|
- byte = (byte << 1) + bit
- end
- buffer << [byte].pack("C")
+ builder = DenseBitmapBuilder.new
+ bitmap.each do |bit|
+ builder.append(bit == 1 ? true : false)
end
Review Comment:
`read_bitmap` now relies on `DenseBitmapBuilder`, but this integration file
doesn't require the new builder. To make it robust to load order (and avoid
`NameError`), ensure the builder file is required before instantiating it.
##########
ruby/red-arrow-format/lib/arrow-format/bitmap-builder.rb:
##########
@@ -0,0 +1,87 @@
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+require_relative "buffer-alignable"
+
+module ArrowFormat
+ class DenseBitmapBuilder
+ include BufferAlignable
+
+ def initialize
+ @buffer = +"".b
+ @n_bits = 0
+ @byte = 0
+ end
+
+ def append(value)
+ @byte |= 1 << @n_bits if value
+ @n_bits += 1
+ flush if @n_bits == 8
+ self
+ end
+
+ def finish
+ flush if @n_bits > 0
+ padding_size = buffer_padding_size(@buffer)
+ @buffer.append_as_bytes(padding(padding_size)) if padding_size > 0
+ @buffer.freeze
+ IO::Buffer.for(@buffer)
Review Comment:
`String#append_as_bytes` is not used anywhere else in this codebase and may
not exist on supported Ruby versions, which would make `finish` crash at
runtime. Use standard `String#<<`/`String#concat` instead.
--
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]
