[GitHub] [arrow] paleolimbot commented on a diff in pull request #19706: ARROW-13858: [R] Create a field ref to a field in a struct

[GitBox <gi...@apache.org>]

paleolimbot commented on code in PR #19706:
URL: https://github.com/apache/arrow/pull/19706#discussion_r1067137683


##########
r/tests/testthat/test-expression.R:
##########
@@ -76,6 +76,16 @@ test_that("Field reference expression schemas and types", {
   expect_equal(x$type(), int32())
 })
 
+test_that("Nested field refs", {
+  x <- Expression$field_ref("x")
+  nested <- x$y
+  expect_r6_class(nested, "Expression")
+  expect_r6_class(x[["y"]], "Expression")
+  expect_r6_class(nested$z, "Expression")
+  # Should this instead be NULL?

Review Comment:
   An error seems safer for now?



##########
r/src/expression.cpp:
##########
@@ -79,6 +110,7 @@ std::shared_ptr<compute::Expression> compute___expr__scalar(
 
 // [[arrow::export]]
 std::string compute___expr__ToString(const std::shared_ptr<compute::Expression>& x) {
+  // TODO: something different if is field ref and IsNested?
   return x->ToString();

Review Comment:
   Is the string representation awful for a nested reference? (Or do we use it to implicitly do stuff?) From the lack of failing tests here, it seems like we don't do a lot of testing of expression objects for whether or not they are field references.



##########
r/tests/testthat/test-dplyr-query.R:
##########
@@ -714,3 +714,48 @@ test_that("Scalars in expressions match the type of the field, if possible", {
     collect()
   expect_equal(result$tpc_h_1, result$as_dbl)
 })
+
+test_that("Can use nested field refs", {
+  nested_data <- tibble(int = 1:5, df_col = tibble(a = 6:10, b = 11:15))
+
+  compare_dplyr_binding(
+    .input %>%
+      mutate(
+        nested = df_col$a,
+        times2 = df_col$a * 2
+      ) %>%
+      filter(nested > 7) %>%
+      collect(),
+    nested_data
+  )
+
+  compare_dplyr_binding(
+    .input %>%
+      mutate(
+        nested = df_col$a,
+        times2 = df_col$a * 2
+      ) %>%
+      filter(nested > 7) %>%
+      summarize(sum(times2)) %>%
+      collect(),
+    nested_data
+  )
+
+  # Now with Dataset

Review Comment:
   Is there a difference in behaviour that would be expected with Dataset? (If so, maybe add a note to that comment and if not, maybe remove it?)



##########
r/src/expression.cpp:
##########
@@ -19,7 +19,7 @@
 
 #include <arrow/compute/api_scalar.h>
 #include <arrow/compute/exec/expression.h>
-
+#include <iostream>

Review Comment:
   Is this addition intentional?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org