From 47b7be4f535c99725b30498d82d77e92a9f183cc Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Sun, 13 Sep 2026 11:56:10 -0400 Subject: [PATCH 1/3] Add validation of udf names --- r/R/udf.R | 27 ++++++++++++++- r/man/register_scalar_function.Rd | 4 ++- r/tests/testthat/_snaps/udf.md | 6 ++++ r/tests/testthat/test-udf.R | 55 +++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 2 deletions(-) diff --git a/r/R/udf.R b/r/R/udf.R index ce7a911e0d97..23023243907e 100644 --- a/r/R/udf.R +++ b/r/R/udf.R @@ -31,7 +31,9 @@ #' for functions with more than one argument. This signature will be used #' to determine if this function is appropriate for a given set of arguments. #' If this function is appropriate for more than one signature, pass a -#' `list()` of the above. +#' `list()` of the above. Arguments are passed to `fun` by position, so if +#' the schema (or field) is named, the names must match the argument names +#' of `fun` (after `context`). #' @param out_type A [DataType] of the output type or a function accepting #' a single argument (`types`), which is a `list()` of [DataType]s. If a #' function it must return a [DataType]. @@ -157,6 +159,29 @@ arrow_scalar_function <- function(fun, in_type, out_type, auto_convert = FALSE) ) } + # Arguments are passed to fun by position, so if the user named the + # fields in in_type, make sure those names line up with fun's arguments + # rather than silently ignoring them (GH-37761) + if (!fun_formals_have_dots) { + fun_arg_names <- names(formals(fun))[-1] + # positions where a named field doesn't match the argument in that position + mismatched <- lapply(in_type, function(sig) { + nms <- names(sig) + same <- nms == fun_arg_names[seq_along(nms)] + same[is.na(same)] <- FALSE + which(nzchar(nms) & !same) + }) + first_bad <- which(lengths(mismatched) > 0)[1] + if (!is.na(first_bad)) { + pos <- mismatched[[first_bad]] + abort(c( + "Names in `in_type` must match the argument names of `fun` (after `context`)", + x = paste0("`in_type` names: ", oxford_paste(names(in_type[[first_bad]])[pos])), + x = paste0("`fun` argument names: ", oxford_paste(fun_arg_names[pos])) + )) + } + } + structure( list( wrapper_fun = wrapper_fun, diff --git a/r/man/register_scalar_function.Rd b/r/man/register_scalar_function.Rd index 6810740194c4..bcec0b2b85bc 100644 --- a/r/man/register_scalar_function.Rd +++ b/r/man/register_scalar_function.Rd @@ -23,7 +23,9 @@ constructed with the expected output type via \code{\link[=as_arrow_array]{as_ar for functions with more than one argument. This signature will be used to determine if this function is appropriate for a given set of arguments. If this function is appropriate for more than one signature, pass a -\code{list()} of the above.} +\code{list()} of the above. Arguments are passed to \code{fun} by position, so if +the schema (or field) is named, the names must match the argument names +of \code{fun} (after \code{context}).} \item{out_type}{A \link{DataType} of the output type or a function accepting a single argument (\code{types}), which is a \code{list()} of \link{DataType}s. If a diff --git a/r/tests/testthat/_snaps/udf.md b/r/tests/testthat/_snaps/udf.md index 89506a7fbc23..deaf54ae64b9 100644 --- a/r/tests/testthat/_snaps/udf.md +++ b/r/tests/testthat/_snaps/udf.md @@ -2,3 +2,9 @@ fun is not a function +# arrow_scalar_function() checks in_type names against fun arguments + + Names in `in_type` must match the argument names of `fun` (after `context`) + x `in_type` names: "blah" and "aj" + x `fun` argument names: "x" and "y" + diff --git a/r/tests/testthat/test-udf.R b/r/tests/testthat/test-udf.R index 2eadd87444b5..e1743de104e0 100644 --- a/r/tests/testthat/test-udf.R +++ b/r/tests/testthat/test-udf.R @@ -323,3 +323,58 @@ test_that("head() on exec plan containing user-defined functions", { expect_equal(nrow(result), 11) }) + +test_that("arrow_scalar_function() checks in_type names against fun arguments", { + # named schema with a different name than the argument + expect_snapshot_error( + arrow_scalar_function( + function(context, x, y) x, + schema(blah = int64(), aj = int64()), + int32() + ) + ) + + # named field with a different name than the argument + expect_error( + arrow_scalar_function( + function(context, x) x, + field("blah", int64()), + int32() + ), + "must match the argument names" + ) + + # partial mismatch across multiple arguments + expect_error( + arrow_scalar_function( + function(context, x, y) x, + schema(x = int32(), b = int32()), + int32() + ), + "must match the argument names" + ) + + # mismatch in a later kernel when registering several at once + expect_error( + arrow_scalar_function( + function(context, x) x, + list(schema(x = int32()), schema(y = int32())), + int32() + ), + "must match the argument names" + ) + + # matching names, unnamed types, and `...` are all still accepted + expect_s3_class( + arrow_scalar_function(function(context, x) x, schema(x = int32()), int32()), + "arrow_scalar_function" + ) + expect_s3_class( + arrow_scalar_function(function(context, anything) anything, int32(), int32()), + "arrow_scalar_function" + ) + expect_s3_class( + arrow_scalar_function(function(...) NULL, schema(blah = int32()), int32()), + "arrow_scalar_function" + ) +}) From 063692f6428298e0334521985dfc3d16ee3816c5 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Sun, 13 Sep 2026 12:03:38 -0400 Subject: [PATCH 2/3] Simplify comparison --- r/R/udf.R | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/r/R/udf.R b/r/R/udf.R index 23023243907e..89d3df15fd45 100644 --- a/r/R/udf.R +++ b/r/R/udf.R @@ -164,20 +164,19 @@ arrow_scalar_function <- function(fun, in_type, out_type, auto_convert = FALSE) # rather than silently ignoring them (GH-37761) if (!fun_formals_have_dots) { fun_arg_names <- names(formals(fun))[-1] - # positions where a named field doesn't match the argument in that position - mismatched <- lapply(in_type, function(sig) { - nms <- names(sig) - same <- nms == fun_arg_names[seq_along(nms)] - same[is.na(same)] <- FALSE - which(nzchar(nms) & !same) - }) - first_bad <- which(lengths(mismatched) > 0)[1] - if (!is.na(first_bad)) { - pos <- mismatched[[first_bad]] + in_type_names <- lapply(in_type, names) + mismatch <- vapply( + in_type_names, + function(nms) { + isTRUE(any(nzchar(nms) & nms != fun_arg_names[seq_along(nms)])) + }, + logical(1) + ) + if (any(mismatch)) { abort(c( "Names in `in_type` must match the argument names of `fun` (after `context`)", - x = paste0("`in_type` names: ", oxford_paste(names(in_type[[first_bad]])[pos])), - x = paste0("`fun` argument names: ", oxford_paste(fun_arg_names[pos])) + x = paste0("`in_type` names: ", oxford_paste(unlist(in_type_names))), + x = paste0("`fun` argument names: ", oxford_paste(fun_arg_names)) )) } } From 73e62ee6bd2fa9dedbc11cccfe8d847b1690bf18 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Thu, 17 Sep 2026 17:47:36 -0500 Subject: [PATCH 3/3] Look at ... --- r/R/udf.R | 48 +++++++++++++++++-------------- r/man/register_scalar_function.Rd | 3 +- r/tests/testthat/_snaps/udf.md | 6 ++++ r/tests/testthat/test-udf.R | 29 ++++++++++++++++--- 4 files changed, 59 insertions(+), 27 deletions(-) diff --git a/r/R/udf.R b/r/R/udf.R index 89d3df15fd45..64fae358f724 100644 --- a/r/R/udf.R +++ b/r/R/udf.R @@ -33,7 +33,8 @@ #' If this function is appropriate for more than one signature, pass a #' `list()` of the above. Arguments are passed to `fun` by position, so if #' the schema (or field) is named, the names must match the argument names -#' of `fun` (after `context`). +#' of `fun` (after `context`). Fields that would be passed to `...` in `fun` +#' can be named anything. #' @param out_type A [DataType] of the output type or a function accepting #' a single argument (`types`), which is a `list()` of [DataType]s. If a #' function it must return a [DataType]. @@ -143,9 +144,12 @@ arrow_scalar_function <- function(fun, in_type, out_type, auto_convert = FALSE) abort("Can't register user-defined scalar function with 0 kernels") } + # All kernels must have the same number of fields (RegisterScalarUDF enforces + # this later), so only the first needs comparing against fun expected_n_args <- in_type[[1]]$num_fields + 1L - fun_formals_have_dots <- any(names(formals(fun)) == "...") - if (!fun_formals_have_dots && length(formals(fun)) != expected_n_args) { + fun_arg_names <- names(formals(fun)) + fun_formals_have_dots <- any(fun_arg_names == "...") + if (!fun_formals_have_dots && length(fun_arg_names) != expected_n_args) { abort( sprintf( paste0( @@ -154,31 +158,31 @@ arrow_scalar_function <- function(fun, in_type, out_type, auto_convert = FALSE) "Did you forget to include `context` as the first argument?" ), expected_n_args, - length(formals(fun)) + length(fun_arg_names) ) ) } # Arguments are passed to fun by position, so if the user named the # fields in in_type, make sure those names line up with fun's arguments - # rather than silently ignoring them (GH-37761) - if (!fun_formals_have_dots) { - fun_arg_names <- names(formals(fun))[-1] - in_type_names <- lapply(in_type, names) - mismatch <- vapply( - in_type_names, - function(nms) { - isTRUE(any(nzchar(nms) & nms != fun_arg_names[seq_along(nms)])) - }, - logical(1) - ) - if (any(mismatch)) { - abort(c( - "Names in `in_type` must match the argument names of `fun` (after `context`)", - x = paste0("`in_type` names: ", oxford_paste(unlist(in_type_names))), - x = paste0("`fun` argument names: ", oxford_paste(fun_arg_names)) - )) - } + # rather than silently ignoring them (GH-37761). Only the arguments + # explicitly named in fun (after `context` and before any `...`) are + # checked: fields beyond those are swallowed by `...` and can be named + # anything. + n_explicit_args <- match("...", fun_arg_names, nomatch = length(fun_arg_names) + 1L) - 1L + explicit_arg_names <- fun_arg_names[seq_len(n_explicit_args)][-1] + in_type_names <- map(in_type, names) + mismatch <- map_lgl(in_type_names, function(nms) { + n_check <- min(length(nms), length(explicit_arg_names)) + nms <- nms[seq_len(n_check)] + any(nzchar(nms) & nms != explicit_arg_names[seq_len(n_check)]) + }) + if (any(mismatch)) { + abort(c( + "Names in `in_type` must match the argument names of `fun` (after `context`)", + x = paste0("`in_type` names: ", oxford_paste(in_type_names[[which(mismatch)[1]]])), + x = paste0("`fun` argument names: ", oxford_paste(explicit_arg_names)) + )) } structure( diff --git a/r/man/register_scalar_function.Rd b/r/man/register_scalar_function.Rd index bcec0b2b85bc..fb43772add8c 100644 --- a/r/man/register_scalar_function.Rd +++ b/r/man/register_scalar_function.Rd @@ -25,7 +25,8 @@ to determine if this function is appropriate for a given set of arguments. If this function is appropriate for more than one signature, pass a \code{list()} of the above. Arguments are passed to \code{fun} by position, so if the schema (or field) is named, the names must match the argument names -of \code{fun} (after \code{context}).} +of \code{fun} (after \code{context}). Fields that would be passed to \code{...} in \code{fun} +can be named anything.} \item{out_type}{A \link{DataType} of the output type or a function accepting a single argument (\code{types}), which is a \code{list()} of \link{DataType}s. If a diff --git a/r/tests/testthat/_snaps/udf.md b/r/tests/testthat/_snaps/udf.md index deaf54ae64b9..be41a3f96904 100644 --- a/r/tests/testthat/_snaps/udf.md +++ b/r/tests/testthat/_snaps/udf.md @@ -8,3 +8,9 @@ x `in_type` names: "blah" and "aj" x `fun` argument names: "x" and "y" +--- + + Names in `in_type` must match the argument names of `fun` (after `context`) + x `in_type` names: "y" + x `fun` argument names: "x" + diff --git a/r/tests/testthat/test-udf.R b/r/tests/testthat/test-udf.R index e1743de104e0..428cc71f88a2 100644 --- a/r/tests/testthat/test-udf.R +++ b/r/tests/testthat/test-udf.R @@ -354,14 +354,14 @@ test_that("arrow_scalar_function() checks in_type names against fun arguments", "must match the argument names" ) - # mismatch in a later kernel when registering several at once - expect_error( + # mismatch in a later kernel when registering several at once: only the + # offending kernel's names are reported + expect_snapshot_error( arrow_scalar_function( function(context, x) x, list(schema(x = int32()), schema(y = int32())), int32() - ), - "must match the argument names" + ) ) # matching names, unnamed types, and `...` are all still accepted @@ -378,3 +378,24 @@ test_that("arrow_scalar_function() checks in_type names against fun arguments", "arrow_scalar_function" ) }) + +test_that("arrow_scalar_function() checks names of explicit arguments before `...`", { + expect_error( + arrow_scalar_function( + function(context, x, ...) x, + schema(blah = int32()), + int32() + ), + "must match the argument names" + ) + + # fields beyond the explicit arguments go into `...` and can be named anything + expect_s3_class( + arrow_scalar_function( + function(context, x, ...) x, + schema(x = int32(), blah = int32()), + int32() + ), + "arrow_scalar_function" + ) +})