Copilot commented on code in PR #50401:
URL: https://github.com/apache/arrow/pull/50401#discussion_r3541570754
##########
ruby/red-arrow-flight/lib/arrow-flight/ticket.rb:
##########
@@ -28,5 +28,12 @@ def try_convert(value)
end
end
end
+
+ alias_method :initialize_raw, :initialize
+ def initialize(data)
Review Comment:
`initialize_raw` is being introduced as a public method here. In the other
Ruby bindings (e.g. ruby/red-arrow/lib/arrow/schema.rb:22-24) the raw
initializer alias is made private to avoid exposing an unintended API surface.
##########
c_glib/test/flight/test-ticket.rb:
##########
@@ -22,23 +22,28 @@ def setup
def test_data
data = "data"
- ticket = ArrowFlight::Ticket.new(data)
+ data_bytes = GLib::Bytes.new(data)
+ ticket = ArrowFlight::Ticket.new(data_bytes)
assert_equal(data,
Review Comment:
This test now passes `GLib::Bytes` directly, which avoids exercising the
regression being fixed (String input + potential GC). Consider keeping the
public API usage (`String`) and forcing a GC cycle to ensure the ticket still
owns the bytes after the original Ruby string becomes unreachable.
##########
c_glib/test/flight/test-criteria.rb:
##########
@@ -22,7 +22,8 @@ def setup
def test_expression
expression = "expression"
- criteria = ArrowFlight::Criteria.new(expression)
+ expression_bytes = GLib::Bytes.new(expression)
+ criteria = ArrowFlight::Criteria.new(expression_bytes)
assert_equal(expression,
Review Comment:
Similar to Ticket, this test now avoids the String input path. Since the PR
changes `ArrowFlight::Criteria.new` to retain the original Ruby object to
prevent GC issues, it would be better for this test to construct from a
`String` and run GC to validate the behavior.
##########
ruby/red-arrow-flight/lib/arrow-flight/criteria.rb:
##########
@@ -0,0 +1,38 @@
+# 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.
+
+module ArrowFlight
+ class Criteria
+ class << self
+ def try_convert(value)
+ case value
+ when String, GLib::Bytes
+ new(value)
+ else
+ nil
+ end
+ end
+ end
+
+ alias_method :initialize_raw, :initialize
+ def initialize(expression)
Review Comment:
`initialize_raw` should be made private to avoid exposing an internal
initializer alias as part of the public API (consistent with other Ruby
bindings such as ruby/red-arrow/lib/arrow/file-output-stream.rb:20-22).
##########
c_glib/test/flight/test-client.rb:
##########
@@ -22,7 +22,7 @@ def setup
@server = nil
omit("Arrow Flight is required") unless defined?(ArrowFlight)
omit("Unstable on Windows") if Gem.win_platform?
- omit("Unstable on x86_64 macOS") if /x86_64-darwin/.match?(RUBY_PLATFORM)
+ omit("Unstable on macOS") if /darwin/.match?(RUBY_PLATFORM)
Review Comment:
This adds a blanket macOS skip to a test that exercises the server
listen/auth-handler path (one of the core areas this PR is trying to make safe
from GC). If the crash only occurs on GitHub Actions runners, consider scoping
the skip to CI and referencing GH-50302 so the test can still run for local
macOS development and remains clearly temporary.
##########
.github/workflows/ruby.yml:
##########
@@ -135,7 +135,7 @@ jobs:
macos:
name: ARM64 macOS GLib & Ruby
- runs-on: macos-latest
+ runs-on: macos-26
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
Review Comment:
The job is labeled as "ARM64 macOS" but `runs-on: macos-26` may schedule an
x86_64 runner depending on GitHub's defaults. If the intent is to reproduce/fix
the ARM64-only crash from GH-50302, consider using the explicit ARM64 label
(e.g. `macos-26-arm64`) or adjusting the job name to match the actual runner
architecture.
--
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]
