Skip to content

Commit 065203d

Browse files
authored
GH-47000: [R] concat_tables on a record_batch causes segfault (#47885)
### Rationale for this change Segfault due to combining recordbatches in concat_tables ### What changes are included in this PR? Convert to tables before combining ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: #47000 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
1 parent b000af1 commit 065203d

File tree

3 files changed

+35
-2
lines changed

3 files changed

+35
-2
lines changed

r/R/table.R

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,8 @@ names.Table <- function(x) x$ColumnNames()
166166
#' does not copy array data, but instead creates new chunked arrays for each
167167
#' column that point at existing array data.
168168
#'
169-
#' @param ... A [Table]
169+
#' @param ... One or more [Table] or [RecordBatch] objects. RecordBatch objects
170+
#' will be automatically converted to Tables.
170171
#' @param unify_schemas If TRUE, the schemas of the tables will be first unified
171172
#' with fields of the same name being merged, then each table will be promoted
172173
#' to the unified schema before being concatenated. Otherwise, all tables should
@@ -176,6 +177,10 @@ names.Table <- function(x) x$ColumnNames()
176177
#' prius <- arrow_table(name = "Prius", mpg = 58, cyl = 4, disp = 1.8)
177178
#' combined <- concat_tables(tbl, prius)
178179
#' tail(combined)$to_data_frame()
180+
#'
181+
#' # Can also pass RecordBatch objects
182+
#' batch <- record_batch(name = "Volt", mpg = 53, cyl = 4, disp = 1.5)
183+
#' combined2 <- concat_tables(tbl, batch)
179184
#' @export
180185
concat_tables <- function(..., unify_schemas = TRUE) {
181186
tables <- list2(...)
@@ -184,6 +189,15 @@ concat_tables <- function(..., unify_schemas = TRUE) {
184189
abort("Must pass at least one Table.")
185190
}
186191

192+
# Convert any RecordBatch objects to Tables
193+
tables <- lapply(tables, function(x) {
194+
if (inherits(x, "RecordBatch")) {
195+
arrow_table(x)
196+
} else {
197+
x
198+
}
199+
})
200+
187201
if (!unify_schemas) {
188202
# assert they have same schema
189203
schema <- tables[[1]]$schema

r/man/concat_tables.Rd

Lines changed: 6 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

r/tests/testthat/test-Table.R

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,20 @@ test_that("Tables can be combined with concat_tables()", {
478478
expect_equal(expected, concat_tables(expected))
479479
})
480480

481+
test_that("concat_tables() handles RecordBatch objects (GH-47000)", {
482+
# concat_tables() should automatically convert RecordBatch to Table
483+
tbl <- arrow_table(a = 1:5, b = letters[1:5])
484+
rb <- record_batch(a = 6:10, b = letters[6:10])
485+
486+
# Concatenating a Table with a RecordBatch should work (not segfault)
487+
result <- concat_tables(tbl, rb)
488+
expect_s3_class(result, "Table")
489+
expect_equal(
490+
result,
491+
arrow_table(a = 1:10, b = letters[1:10])
492+
)
493+
})
494+
481495
test_that("Table supports rbind", {
482496
expect_error(
483497
rbind(arrow_table(a = 1:10), arrow_table(a = c("a", "b"))),

0 commit comments

Comments
 (0)