Copilot commented on code in PR #651:
URL: https://github.com/apache/sedona-db/pull/651#discussion_r2835926865
##########
r/sedonadb/tests/testthat/test-expression.R:
##########
@@ -58,6 +58,32 @@ test_that("function calls with a translation become function
calls", {
expect_snapshot(sd_eval_expr(quote(base::abs(-1L))))
})
+test_that("function calls explicitly referencing DataFusion functions work", {
+ # Scalar function
+ expect_snapshot(sd_eval_expr(quote(.fns$abs(-1L))))
+
+ # Aggregate function
+ expect_snapshot(sd_eval_expr(quote(.fns$sum(-1L))))
+ expect_snapshot(sd_eval_expr(quote(.fns$sum(-1L, na.rm = TRUE))))
+ expect_snapshot(sd_eval_expr(quote(.fns$sum(-1L, na.rm = FALSE))))
+
+ # Check for a reasonable error if this is not a valid name or we have
+ # named arguments
+ expect_snapshot_error(sd_eval_expr(quote(.fns$absolutely_not(-1L))))
+ expect_snapshot_error(sd_eval_expr(quote(.fns$absolutely_not(x = -1L))))
+})
+
+test_that("function calls referencing SedonaDB SQL functions work", {
+ expect_snapshot(sd_eval_expr(quote(st_point(0, 1))))
+ expect_snapshot(sd_eval_expr(quote(st_envelope_agg(st_point(0, 1)))))
+ expect_snapshot(
+ sd_eval_expr(quote(st_envelope_agg(st_point(0, 1), na.rm = TRUE)))
+ )
+
+ # Check for a reasonable errors (named arguments are not allowed)
Review Comment:
The comment has a grammatical error: "Check for a reasonable errors" should
be "Check for reasonable errors" or "Check for a reasonable error".
```suggestion
# Check for reasonable errors (named arguments are not allowed)
```
##########
r/sedonadb/R/dataframe.R:
##########
@@ -333,6 +498,9 @@ sd_write_parquet <- function(
overwrite_bbox_columns = FALSE
) {
.data <- as_sedonadb_dataframe(.data)
+ if (!is.null(.data$group_by)) {
+ stop("sd_select() does not support grouped input")
Review Comment:
The error message says "sd_select() does not support grouped input" but this
is in the `sd_write_parquet()` function. The error message should be updated to
say "sd_write_parquet() does not support grouped input".
```suggestion
stop("sd_write_parquet() does not support grouped input")
```
##########
r/sedonadb/R/expression.R:
##########
@@ -200,6 +278,26 @@ sd_eval_default <- function(expr, expr_ctx) {
rlang::eval_tidy(expr, data = expr_ctx$data, env = expr_ctx$env)
}
+# Needed for sd_arrange(), as wrapping expression in desc() is how a descending
+# sort order is specified. Unwraps desc(inner_expr) to separate the
expressions.
+unwrap_desc <- function(exprs) {
+ inner_exprs <- vector("list", length(exprs))
+ is_descending <- vector("logical", length(exprs))
+ for (i in seq_along(exprs)) {
+ expr <- exprs[[i]]
+
+ if (rlang::is_call(expr, "desc") || rlang::is_call("desc", ns = "dplyr")) {
Review Comment:
The second condition in the OR statement has incorrect argument order.
`rlang::is_call()` expects the expression as the first argument, but `"desc"`
is passed as the first argument instead. This should be `rlang::is_call(expr,
"desc", ns = "dplyr")` to check if the expression is a call to `dplyr::desc`.
```suggestion
if (rlang::is_call(expr, "desc") || rlang::is_call(expr, "desc", ns =
"dplyr")) {
```
##########
r/sedonadb/R/expression.R:
##########
@@ -265,6 +402,12 @@ ensure_translations_registered <- function() {
return()
}
+ # Register default translations for our st_, sd_, and rs_ functions
+ fn_names <- utils::.DollarNames(.fns, "^(st|rs|sd|)_")
Review Comment:
The regex pattern `"^(st|rs|sd|)_"` contains an empty alternative before the
underscore, which means it would match function names that start with just `_`
(e.g., `_foo`). This is likely unintentional. Consider using `"^(st|rs|sd)_"`
instead to only match functions starting with `st_`, `rs_`, or `sd_`.
```suggestion
fn_names <- utils::.DollarNames(.fns, "^(st|rs|sd)_")
```
--
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]
