[GitHub] [arrow] nealrichardson commented on a change in pull request #10647: ARROW-13174: [C++][Compute] Add strftime kernel

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

nealrichardson commented on a change in pull request #10647:
URL: https://github.com/apache/arrow/pull/10647#discussion_r676641413



##########
File path: r/src/compute.cpp
##########
@@ -288,6 +288,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
         cpp11::as_cpp<arrow::TimeUnit::type>(options["unit"]));
   }
 
+  if (func_name == "strftime") {
+    using Options = arrow::compute::StrftimeOptions;
+    std::string format = "%Y-%m-%dT%H:%M:%SZ";
+    std::string locale = "C";
+    if (!Rf_isNull(options["format"])) {
+      format = cpp11::as_cpp<std::string>(options["format"]);
+    }
+    if (!Rf_isNull(options["locale"])) {
+      locale = cpp11::as_cpp<std::string>(options["locale"]);
+    }
+    return std::make_shared<Options>(Options(format, locale));
+  }
+

Review comment:
       Thanks for adding this, but since there aren't any tests that exercise this I don't think we're done with the R bindings. I've made ARROW-13448 for that.

##########
File path: r/configure.win
##########
@@ -44,7 +44,7 @@ else
   RWINLIB="../windows/$(ls windows/ | grep ^arrow- | tail -n 1)"
 fi
 OPENSSL_LIBS="-lcrypto -lcrypt32"
-MIMALLOC_LIBS="-lbcrypt -lpsapi"
+MIMALLOC_LIBS="-lbcrypt -lpsapi -lole32"

Review comment:
       This isn't the right place to put it because it's not about mimalloc. I think L53 is where it belongs




-- 
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