jonkeane commented on code in PR #49276: URL: https://github.com/apache/arrow/pull/49276#discussion_r3499368119
########## r/R/ipc-stream.R: ########## @@ -20,22 +20,21 @@ #' Apache Arrow defines two formats for [serializing data for interprocess #' communication #' (IPC)](https://arrow.apache.org/docs/format/Columnar.html#serialization-and-interprocess-communication-ipc): -#' a "stream" format and a "file" format, known as Feather. `write_ipc_stream()` -#' and [write_feather()] write those formats, respectively. +#' a "stream" format and a "file" format. `write_ipc_stream()` +#' and [write_ipc_file()] write those formats, respectively. #' -#' @inheritParams write_feather -#' @param ... extra parameters passed to `write_feather()`. +#' @inheritParams write_ipc_file #' #' @return `x`, invisibly. -#' @seealso [write_feather()] for writing IPC files. [write_to_raw()] to +#' @seealso [write_ipc_file()] for writing IPC files. [write_to_raw()] to #' serialize data to a buffer. #' [RecordBatchWriter] for a lower-level interface. #' @export #' @examples #' tf <- tempfile() #' on.exit(unlink(tf)) #' write_ipc_stream(mtcars, tf) -write_ipc_stream <- function(x, sink, ...) { +write_ipc_stream <- function(x, sink) { Review Comment: We might have had this in here because people _used to have_ other arguments that they were passing and we didn't want to break on them. I haven't looked through git history, but that might be something to send a bot after to confirm. Maybe instead of removing them we could accept them and add a deprecation notice that we will be removing them for anyone who might (accidentally) be using them? ########## r/R/ipc-stream.R: ########## @@ -89,18 +88,17 @@ write_to_raw <- function(x, format = c("stream", "file")) { #' open. #' @param as_data_frame Should the function return a `tibble` (default) or #' an Arrow [Table]? -#' @param ... extra parameters passed to `read_feather()`. #' #' @return A `tibble` if `as_data_frame` is `TRUE` (the default), or an #' Arrow [Table] otherwise -#' @seealso [write_feather()] for writing IPC files. [RecordBatchReader] for a +#' @seealso [write_ipc_file()] for writing IPC files. [RecordBatchReader] for a #' lower-level interface. #' @section Untrusted data: #' If reading from an untrusted source, you can validate the data by reading #' with `as_data_frame = FALSE` and calling `$ValidateFull()` on the Table #' before processing. #' @export -read_ipc_stream <- function(file, as_data_frame = TRUE, ...) { +read_ipc_stream <- function(file, as_data_frame = TRUE) { Review Comment: Same thing here ########## r/R/dataset-format.R: ########## @@ -97,9 +101,7 @@ FileFormat$create <- function(format, schema = NULL, partitioning = NULL, ...) { #' @export as.character.FileFormat <- function(x, ...) { - out <- x$type - # Slight hack: special case IPC -> feather, otherwise is just the type_name - ifelse(out == "ipc", "feather", out) + x$type } Review Comment: Ha now this could be a one liner! ########## r/R/dataset.R: ########## @@ -481,7 +481,7 @@ FileSystemDataset <- R6Class( file_type <- self$format$type pretty_file_type <- list( parquet = "Parquet", - ipc = "Feather" + ipc = "IPC" Review Comment: I'm slightly surprised by the all caps here? Is that just the display name? And `"ipc"` is still what we expect people to type? -- 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]
