You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Reranko05 (via GitHub)" <gi...@apache.org> on 2026/06/19 10:03:37 UTC
[PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Reranko05 opened a new pull request, #50226:
URL: https://github.com/apache/arrow/pull/50226
### Rationale for this change
`Arrow::Table#merge` and `Arrow::RecordBatch#merge` had similar implementations, resulting in duplicated merge logic.
### What changes are included in this PR?
This PR moves the common merge implementation to `Arrow::ColumnContainable`.
Container-specific behavior such as column conversion and creating the merged container remains implemented in `Arrow::Table` and `Arrow::RecordBatch`.
### Are these changes tested?
Yes.
The existing `RecordBatch` and `Table` test suites were run and passed successfully.
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3441837409
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -135,6 +78,20 @@ def method_missing(name, *args, &block)
private
+ def merge_target_class
+ RecordBatch
+ end
+
+ def create_merged_container(fields, arrays)
+ record_batch = self.class.new(
+ Schema.new(fields),
+ n_rows,
+ arrays,
+ )
Review Comment:
I investigated using `RecordBatch.new(Schema.new(fields), arrays)` to unify this code, but it uses the `RecordBatchBuilder` path and fails for the merged arrays. It looks like `RecordBatch.new(schema, n_rows, arrays)` is required here, so I kept the current implementation.
If you have any other way in mind to unify this code, let me know.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3449893775
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -37,7 +37,12 @@ def new(*args)
super(schema, n_rows, values)
when 2
schema, data = args
- RecordBatchBuilder.build(schema, data)
+ schema = Schema.new(schema) unless schema.is_a?(Schema)
+ if !data.empty? && data.all? {|array| array.is_a?(Arrow::Array)}
Review Comment:
Addressed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #50226:
URL: https://github.com/apache/arrow/pull/50226#issuecomment-4766376852
Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #50226:
URL: https://github.com/apache/arrow/pull/50226
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50226:
URL: https://github.com/apache/arrow/pull/50226#issuecomment-4750551254
:warning: GitHub issue #50225 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3449975055
##########
ruby/red-arrow/lib/arrow/table.rb:
##########
@@ -628,6 +574,7 @@ def method_missing(name, *args, &block)
end
private
+
Review Comment:
Good catch.. updated
##########
ruby/red-arrow/lib/arrow/column-containable.rb:
##########
@@ -152,5 +152,76 @@ def [](selector)
def column_names
@column_names ||= columns.collect(&:name)
end
+
+ # Merges columns from the given container or Hash and creates
+ # a new container.
+ #
+ # @param other [Hash, self]
+ # The columns to be merged.
+ #
+ # @return [self]
+ def merge(other)
+ added_columns = {}
+ removed_columns = {}
+
+ case other
+ when Hash
+ other.each do |name, value|
+ name = name.to_s
+ if value
+ added_columns[name] = ensure_raw_column(name, value)
+ else
+ removed_columns[name] = true
+ end
+ end
+ when self.class
+ other.columns.each do |column|
+ name = column.name
+ added_columns[name] = ensure_raw_column(name, column)
+ end
+ else
+ message = "merge target must be Hash or #{self.class}: " +
+ "<#{other.inspect}>: #{inspect}"
+ raise ArgumentError, message
+ end
+
+ new_columns = []
+
+ columns.each do |column|
+ column_name = column.name
+ new_column = added_columns.delete(column_name)
+
+ if new_column
+ new_columns << new_column
+ next
+ end
+
+ next if removed_columns.key?(column_name)
+
+ new_columns << ensure_raw_column(column_name, column)
+ end
+
+ added_columns.each_value do |new_column|
+ new_columns << new_column
+ end
+
+ new_fields = []
+ new_arrays = []
+
+ new_columns.each do |new_column|
+ new_fields << new_column[:field]
+ new_arrays << new_column[:data]
+ end
+
+ create_merged_container(new_fields, new_arrays)
+ end
+
+ private
+
+ def create_merged_container(fields, arrays)
+ container = self.class.new(fields, arrays)
+ share_input(container)
+ container
Review Comment:
Addressed
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3447910779
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -135,6 +78,20 @@ def method_missing(name, *args, &block)
private
+ def merge_target_class
+ RecordBatch
+ end
+
+ def create_merged_container(fields, arrays)
+ record_batch = self.class.new(
+ Schema.new(fields),
+ n_rows,
+ arrays,
+ )
Review Comment:
Updated. Thanks for the suggestion. `RecordBatch.new(schema, arrays)` now handles arrays directly, so `create_merged_container` can use it without passing `n_rows`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3449890503
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -135,6 +83,15 @@ def method_missing(name, *args, &block)
private
+ def create_merged_container(fields, arrays)
+ record_batch = self.class.new(
+ Schema.new(fields),
+ arrays,
+ )
+ share_input(record_batch)
+ record_batch
+ end
+
Review Comment:
Addressed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "Reranko05 (via GitHub)" <gi...@apache.org>.
Reranko05 commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3441830390
##########
ruby/red-arrow/lib/arrow/column-containable.rb:
##########
@@ -152,5 +152,68 @@ def [](selector)
def column_names
@column_names ||= columns.collect(&:name)
end
+
+ # Merges columns from the given container or Hash and creates
+ # a new container.
+ #
+ # @param other [Hash, self]
+ # The columns to be merged.
+ #
+ # @return [self]
+ def merge(other)
+ added_columns = {}
+ removed_columns = {}
+
+ case other
+ when Hash
+ other.each do |name, value|
+ name = name.to_s
+ if value
+ added_columns[name] = ensure_raw_column(name, value)
+ else
+ removed_columns[name] = true
+ end
+ end
+ when merge_target_class
Review Comment:
Updated
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3449285193
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -37,7 +37,12 @@ def new(*args)
super(schema, n_rows, values)
when 2
schema, data = args
- RecordBatchBuilder.build(schema, data)
+ schema = Schema.new(schema) unless schema.is_a?(Schema)
+ if !data.empty? && data.all? {|array| array.is_a?(Arrow::Array)}
Review Comment:
Could you use `and` not `&&` in this code base?
```suggestion
if !data.empty? and data.all? {|array| array.is_a?(Arrow::Array)}
```
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -135,6 +83,15 @@ def method_missing(name, *args, &block)
private
+ def create_merged_container(fields, arrays)
+ record_batch = self.class.new(
+ Schema.new(fields),
+ arrays,
+ )
+ share_input(record_batch)
+ record_batch
+ end
+
Review Comment:
Can we move this to `column_containable.rb` now?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3447695008
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -135,6 +78,20 @@ def method_missing(name, *args, &block)
private
+ def merge_target_class
+ RecordBatch
+ end
+
+ def create_merged_container(fields, arrays)
+ record_batch = self.class.new(
+ Schema.new(fields),
+ n_rows,
+ arrays,
+ )
Review Comment:
Does this work?
```diff
diff --git a/ruby/red-arrow/lib/arrow/record-batch.rb b/ruby/red-arrow/lib/arrow/record-batch.rb
index 7a9ea296ea..0a4f09c864 100644
--- a/ruby/red-arrow/lib/arrow/record-batch.rb
+++ b/ruby/red-arrow/lib/arrow/record-batch.rb
@@ -37,7 +37,12 @@ module Arrow
super(schema, n_rows, values)
when 2
schema, data = args
- RecordBatchBuilder.build(schema, data)
+ schema = Schema.new(schema) unless schema.is_a?(Schema)
+ if data.all? {|array| array.is_a?(Array)}
+ super(schema, data[0].size, data)
+ else
+ RecordBatchBuilder.build(schema, data)
+ end
when 3
super
else
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3441760975
##########
ruby/red-arrow/lib/arrow/column-containable.rb:
##########
@@ -152,5 +152,68 @@ def [](selector)
def column_names
@column_names ||= columns.collect(&:name)
end
+
+ # Merges columns from the given container or Hash and creates
+ # a new container.
+ #
+ # @param other [Hash, self]
+ # The columns to be merged.
+ #
+ # @return [self]
+ def merge(other)
+ added_columns = {}
+ removed_columns = {}
+
+ case other
+ when Hash
+ other.each do |name, value|
+ name = name.to_s
+ if value
+ added_columns[name] = ensure_raw_column(name, value)
+ else
+ removed_columns[name] = true
+ end
+ end
+ when merge_target_class
Review Comment:
Can we use `self.class` here?
```suggestion
when self.class
```
##########
ruby/red-arrow/lib/arrow/record-batch.rb:
##########
@@ -135,6 +78,20 @@ def method_missing(name, *args, &block)
private
+ def merge_target_class
+ RecordBatch
+ end
+
+ def create_merged_container(fields, arrays)
+ record_batch = self.class.new(
+ Schema.new(fields),
+ n_rows,
+ arrays,
+ )
Review Comment:
If we can use `RecordBatch.new(fields, arrays)`, can we unify this code?
If so, let's accept `RecordBatch.new(fields, arrays)`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #50226:
URL: https://github.com/apache/arrow/pull/50226#discussion_r3449907169
##########
ruby/red-arrow/lib/arrow/column-containable.rb:
##########
@@ -152,5 +152,76 @@ def [](selector)
def column_names
@column_names ||= columns.collect(&:name)
end
+
+ # Merges columns from the given container or Hash and creates
+ # a new container.
+ #
+ # @param other [Hash, self]
+ # The columns to be merged.
+ #
+ # @return [self]
+ def merge(other)
+ added_columns = {}
+ removed_columns = {}
+
+ case other
+ when Hash
+ other.each do |name, value|
+ name = name.to_s
+ if value
+ added_columns[name] = ensure_raw_column(name, value)
+ else
+ removed_columns[name] = true
+ end
+ end
+ when self.class
+ other.columns.each do |column|
+ name = column.name
+ added_columns[name] = ensure_raw_column(name, column)
+ end
+ else
+ message = "merge target must be Hash or #{self.class}: " +
+ "<#{other.inspect}>: #{inspect}"
+ raise ArgumentError, message
+ end
+
+ new_columns = []
+
+ columns.each do |column|
+ column_name = column.name
+ new_column = added_columns.delete(column_name)
+
+ if new_column
+ new_columns << new_column
+ next
+ end
+
+ next if removed_columns.key?(column_name)
+
+ new_columns << ensure_raw_column(column_name, column)
+ end
+
+ added_columns.each_value do |new_column|
+ new_columns << new_column
+ end
+
+ new_fields = []
+ new_arrays = []
+
+ new_columns.each do |new_column|
+ new_fields << new_column[:field]
+ new_arrays << new_column[:data]
+ end
+
+ create_merged_container(new_fields, new_arrays)
+ end
+
+ private
+
+ def create_merged_container(fields, arrays)
+ container = self.class.new(fields, arrays)
+ share_input(container)
+ container
Review Comment:
Can we merge them?
```suggestion
merged = self.class.new(new_fields, new_arrays)
share_input(merged)
merged
```
##########
ruby/red-arrow/lib/arrow/table.rb:
##########
@@ -628,6 +574,7 @@ def method_missing(name, *args, &block)
end
private
+
Review Comment:
Could you revert needless changes?
```suggestion
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50225: [Ruby] Move merge implementation to ColumnContainable [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50226:
URL: https://github.com/apache/arrow/pull/50226#issuecomment-4769962162
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0249a2e4ca1f159108359cdae8b8f7f2c8eca557.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/82752660844) has more details. It also includes information about 38 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org