Skip to content

GH-50225: [Ruby] Move merge implementation to ColumnContainable#50226

Open
Reranko05 wants to merge 1 commit into
apache:mainfrom
Reranko05:column-containable-merge-refactor
Open

GH-50225: [Ruby] Move merge implementation to ColumnContainable#50226
Reranko05 wants to merge 1 commit into
apache:mainfrom
Reranko05:column-containable-merge-refactor

Conversation

@Reranko05

@Reranko05 Reranko05 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

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.

@Reranko05 Reranko05 requested a review from kou as a code owner June 19, 2026 10:03
Copilot AI review requested due to automatic review settings June 19, 2026 10:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50225 has been automatically assigned in GitHub to PR creator.

Comment thread ruby/red-arrow/lib/arrow/column-containable.rb Outdated
Comment on lines +86 to +90
record_batch = self.class.new(
Schema.new(fields),
n_rows,
arrays,
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can use RecordBatch.new(fields, arrays), can we unify this code?

If so, let's accept RecordBatch.new(fields, arrays).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 19, 2026
@Reranko05 Reranko05 force-pushed the column-containable-merge-refactor branch from 32bc7e4 to cef651f Compare June 19, 2026 10:24
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 19, 2026
@Reranko05 Reranko05 requested a review from kou June 19, 2026 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants