You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/11/10 18:39:43 UTC
[GitHub] [arrow] dragosmg opened a new pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
dragosmg opened a new pull request #11668:
URL: https://github.com/apache/arrow/pull/11668
@thisisnic @jonkeane & @paleolimbot This is my first attempt at the task. Not convinced this is the best approach for exposing the args to the user (in prep for hooking them up to the C++ functionality).
I plan on adding more unit test for this PR.
--
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
[GitHub] [arrow] jonkeane commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r754729253
##########
File path: r/R/csv.R
##########
@@ -624,8 +636,27 @@ readr_to_csv_convert_options <- function(na,
write_csv_arrow <- function(x,
sink,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+
Review comment:
```suggestion
```
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r761029566
##########
File path: r/R/csv.R
##########
@@ -415,6 +415,16 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
)
}
+readr_to_csv_write_options <- function(include_header,
+ batch_size = 1024L) {
+ assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
Review comment:
Should we aim for a more informative error message here?
```
> write_csv_arrow(my_tibble, csv_file, batch_size = Inf)
Error: is_integerish(x = batch_size, n = 1, finite = TRUE) is not TRUE
```
might not be the easiest to digest by a user encountering `rlang::is_integerish()` for the first time. We also offer no direction for further action.
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-986995302
Benchmark runs are scheduled for baseline = 18b4991dde2fa654a732f35463995231f8eb5f35 and contender = 85b276dd13512d6676a0bc2dc703a0f9b6934236. 85b276dd13512d6676a0bc2dc703a0f9b6934236 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bf1b67699e7b4a94b505d7bd7cb7b436...f3a38d5da3d04da6be80e3aa7b64034f/)
[Failed :arrow_down:0.9% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ee6025c6d4d47689af4b410f66912eb...f4b005e54b794f658e3fb99a44955bac/)
[Finished :arrow_down:0.31% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2fa451c6124e4463812d9f4f72f33df9...cc1852af4b9c4246b71b112eab70bfb7/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-986995302
Benchmark runs are scheduled for baseline = 18b4991dde2fa654a732f35463995231f8eb5f35 and contender = 85b276dd13512d6676a0bc2dc703a0f9b6934236. 85b276dd13512d6676a0bc2dc703a0f9b6934236 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bf1b67699e7b4a94b505d7bd7cb7b436...f3a38d5da3d04da6be80e3aa7b64034f/)
[Scheduled :warning: ursa-i9-9960x is offline.] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ee6025c6d4d47689af4b410f66912eb...f4b005e54b794f658e3fb99a44955bac/)
[Finished :arrow_down:0.31% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2fa451c6124e4463812d9f4f72f33df9...cc1852af4b9c4246b71b112eab70bfb7/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r752133069
##########
File path: r/R/csv.R
##########
@@ -612,6 +622,8 @@ readr_to_csv_convert_options <- function(na,
#' system (`SubTreeFileSystem`)
#' @param include_header Whether to write an initial header line with column names
#' @param batch_size Maximum number of rows processed at a time. Default is 1024.
+#' @param write_options read_options see [file reader options][CsvWriteOptions]
Review comment:
The downside to this is that we do not have much in terms of content for `CsvWriteOptions`. We direct to it from a couple of places, but maybe we need to extent the documentation, more so once we surface more options from the C++ code.
--
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
[GitHub] [arrow] dragosmg commented on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-981787035
I have created a series of follow-up Jiras:
* [ARROW-14902](https://issues.apache.org/jira/browse/ARROW-14902) - umbrella ticket to update `write_csv_arrow()` once the C++ functionality is there.
* [ARROW-14903](https://issues.apache.org/jira/browse/ARROW-14903) - control the string used for missing data
* [ARROW-14904](https://issues.apache.org/jira/browse/ARROW-14904) - enable append/overwrite
* [ARROW-14905](https://issues.apache.org/jira/browse/ARROW-14905) - control quoting
* [ARROW-14906](https://issues.apache.org/jira/browse/ARROW-14906) - control the type of escape used for quoting
* [ARROW-14907](https://issues.apache.org/jira/browse/ARROW-14907) - control the end-of-line character
--
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
[GitHub] [arrow] paleolimbot commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r747439162
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
I think you might be better off with something like:
``` r
write_csv_arrow <- function(x, file, the = "options", you = "do", support = "right now", ..., sink = "DEPRECTED") {
unsupported_passed_args <- names(list(...))
# handle difference in name between 'file' and 'sink'
# convert write_csv() names/values to the names/values you need for `CsvWriteOptions$create()`
# ...
}
```
(a little simpler and doesn't assume that the names of `CsvWriteOptions::create()` are going to be the names that readr chose for write_csv`)
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758548718
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,58 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ paste("You have supplied both \"file\" and \"sink\" arguments. Please",
+ "supply only one of them."),
+ call. = FALSE
+ )
Review comment:
Can simplify this by removing the `paste()` since `stop()` already pastes together its arguments (with no separator)
```suggestion
stop(
"You have supplied both \"file\" and \"sink\" arguments. ",
"Please supply only one of them.",
call. = FALSE
)
```
--
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
[GitHub] [arrow] jonkeane commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758638024
##########
File path: r/R/csv.R
##########
@@ -415,6 +415,16 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
)
}
+readr_to_csv_write_options <- function(include_header,
+ batch_size = 1024L) {
+ assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
Review comment:
This is more a curiosity than anything else: what _would_ happen if I gave an infinite batch size? I could imagine someone doing that if they absolutely needed everything to we written in one batch but somehow couldn't know the nrows before hand)
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,57 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ "You have supplied both \"file\" and \"sink\" arguments. Please ",
+ "supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ "You have supplied both \"col_names\" and \"include_header\" ",
+ "arguments. Please supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ # default value are considered missing by base R
+ if (missing(include_header) && !missing(col_names)) {
+ message(
+ "You have supplied a value for \"col_names\". This will overwrite ",
+ "the value for the \"include_headers\" argument.")
+ include_header <- col_names
+ }
Review comment:
Am I reading this right that you get here if you specify `write_csv_arrow(df, "file.csv", col_names = TRUE)`?
If so, I don't think we need a message there about replacing `include_header` since it's not being specified / there's no conflict.
--
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
[GitHub] [arrow] ursabot commented on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-986995302
Benchmark runs are scheduled for baseline = 18b4991dde2fa654a732f35463995231f8eb5f35 and contender = 85b276dd13512d6676a0bc2dc703a0f9b6934236. 85b276dd13512d6676a0bc2dc703a0f9b6934236 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bf1b67699e7b4a94b505d7bd7cb7b436...f3a38d5da3d04da6be80e3aa7b64034f/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ee6025c6d4d47689af4b410f66912eb...f4b005e54b794f658e3fb99a44955bac/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2fa451c6124e4463812d9f4f72f33df9...cc1852af4b9c4246b71b112eab70bfb7/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758551588
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,58 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ paste("You have supplied both \"file\" and \"sink\" arguments. Please",
+ "supply only one of them."),
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ paste("You have supplied both \"col_names\" and \"include_header\"",
+ "arguments. Please supply only one of them."),
+ call. = FALSE
+ )
+ }
+
+ # default value are considered missing by base R
+ if (missing(include_header) && !missing(col_names)) {
+ message(
+ paste(
+ "You have supplied a value for \"col_names\". This will overwrite",
+ "the value for the \"include_headers\" argument."))
Review comment:
`message()` pastes together its arguments (with no separator)
```suggestion
message(
"You have supplied a value for \"col_names\". This will overwrite ",
"the value for the \"include_headers\" argument."
)
```
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-986995302
Benchmark runs are scheduled for baseline = 18b4991dde2fa654a732f35463995231f8eb5f35 and contender = 85b276dd13512d6676a0bc2dc703a0f9b6934236. 85b276dd13512d6676a0bc2dc703a0f9b6934236 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bf1b67699e7b4a94b505d7bd7cb7b436...f3a38d5da3d04da6be80e3aa7b64034f/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ee6025c6d4d47689af4b410f66912eb...f4b005e54b794f658e3fb99a44955bac/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2fa451c6124e4463812d9f4f72f33df9...cc1852af4b9c4246b71b112eab70bfb7/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758612481
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,58 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ paste("You have supplied both \"file\" and \"sink\" arguments. Please",
+ "supply only one of them."),
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ paste("You have supplied both \"col_names\" and \"include_header\"",
+ "arguments. Please supply only one of them."),
+ call. = FALSE
+ )
Review comment:
Done
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-965631627
https://issues.apache.org/jira/browse/ARROW-13623
--
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
[GitHub] [arrow] jonkeane commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r748380013
##########
File path: r/tests/testthat/test-csv.R
##########
@@ -378,3 +378,10 @@ test_that("time mapping work as expected (ARROW-13624)", {
expect_equal(df, tbl, ignore_attr = "tzone")
})
+
+test_that("Writing a CSV errors when unsupported (yet) readr args are used", {
+ expect_error(
+ write_csv_arrow(tbl, csv_file, append = FALSE),
+ "The following argument is not yet supported in Arrow: \"append\""
+ )
+})
Review comment:
Thanks for proactively adding the error test, that's great!
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
Yeah, I like this approach proposed by @paleolimbot here. It should also avoid any need for parsing the formals and comparing those.
You can also make a helper function for that [like we do with the reading side](readr_to_csv_parse_options)
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r748413837
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
The link is broken, but I think I know what you mean. That was one of my other possible directions. Cool. Off to do some work and will update the PR.
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r761009126
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,57 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ "You have supplied both \"file\" and \"sink\" arguments. Please ",
+ "supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ "You have supplied both \"col_names\" and \"include_header\" ",
+ "arguments. Please supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ # default value are considered missing by base R
+ if (missing(include_header) && !missing(col_names)) {
+ message(
+ "You have supplied a value for \"col_names\". This will overwrite ",
+ "the value for the \"include_headers\" argument.")
+ include_header <- col_names
+ }
Review comment:
~The use case I had in mind was mostly how to deal with `col_names = FALSE`.
I'm trying to figure what would be the best way to deal with this. `include_header` is `TRUE` by default. The main use case for the message was to warn when the user tries to pass a different / contradictory value for `col_names` which overwrites the default value for `include_header`. I could just make the message appear then `col_names == FALSE`.~
--
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
[GitHub] [arrow] jonkeane closed pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #11668:
URL: https://github.com/apache/arrow/pull/11668
--
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
[GitHub] [arrow] thisisnic commented on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-966195629
Did you say you'd opened some JIRA tickets for implementing the missing functionality? If so, how about popping links to those in some comments, and then it'll be more apparent what the dependencies are for implementing the new functionality?
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r761016492
##########
File path: r/R/csv.R
##########
@@ -415,6 +415,16 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
)
}
+readr_to_csv_write_options <- function(include_header,
+ batch_size = 1024L) {
+ assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
Review comment:
If we disabled that check / assertion in both `readr_to_csv_write_options` and `CsvWriteOptions$create()`, we would surface the following error message from C++:
```
Error: Invalid: WriteOptions: batch_size must be at least 1: -2147483648
/Users/dragos/Documents/arrow/cpp/src/arrow/csv/writer.cc:291 options.Validate()
/Users/dragos/Documents/arrow/cpp/src/arrow/csv/writer.cc:433 MakeCSVWriter(output, table.schema(), options)
In addition: Warning message:
In csv___WriteOptions__initialize(list(include_header = include_header, :
NAs introduced by coercion to integer range
```
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r756172067
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
Done. @jonkeane please have a look to see if what I did makes sense. I think we could improve the arg description and maybe not `message()` about the whole `sink` `file` situation?
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r752092203
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
@jonkeane & @paleolimbot do we change the name of the write_csv_arrow() argument from sink to file? What about include_header to col_names?
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-986995302
Benchmark runs are scheduled for baseline = 18b4991dde2fa654a732f35463995231f8eb5f35 and contender = 85b276dd13512d6676a0bc2dc703a0f9b6934236. 85b276dd13512d6676a0bc2dc703a0f9b6934236 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bf1b67699e7b4a94b505d7bd7cb7b436...f3a38d5da3d04da6be80e3aa7b64034f/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/2ee6025c6d4d47689af4b410f66912eb...f4b005e54b794f658e3fb99a44955bac/)
[Finished :arrow_down:0.31% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2fa451c6124e4463812d9f4f72f33df9...cc1852af4b9c4246b71b112eab70bfb7/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] dragosmg commented on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-966196975
No tickets just yet. It's not 100% clear how close we want to end up to the `readr::write_csv()` signature - e.g. probably we won't be implementing `num_threads`.
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r761009126
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,57 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ "You have supplied both \"file\" and \"sink\" arguments. Please ",
+ "supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ "You have supplied both \"col_names\" and \"include_header\" ",
+ "arguments. Please supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ # default value are considered missing by base R
+ if (missing(include_header) && !missing(col_names)) {
+ message(
+ "You have supplied a value for \"col_names\". This will overwrite ",
+ "the value for the \"include_headers\" argument.")
+ include_header <- col_names
+ }
Review comment:
I'm trying to figure what would be the best way to deal with this. `include_header` is `TRUE` by default. The main use case for the message was to warn when the user tries to pass a different / contradictory value for `col_names` which overwrites the default value for `include_header`. I could just make the message appear then `col_names == FALSE`.
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r752133069
##########
File path: r/R/csv.R
##########
@@ -612,6 +622,8 @@ readr_to_csv_convert_options <- function(na,
#' system (`SubTreeFileSystem`)
#' @param include_header Whether to write an initial header line with column names
#' @param batch_size Maximum number of rows processed at a time. Default is 1024.
+#' @param write_options read_options see [file reader options][CsvWriteOptions]
Review comment:
The downside to this approach is that we do not have much in terms of content for `CsvWriteOptions`. We direct to it from a couple of places, but maybe we need to extent the documentation, more so once we surface more options from the C++ code. Happy to open a follow-up Jira and update the `CsvWriteOptions` documentation.
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r752133069
##########
File path: r/R/csv.R
##########
@@ -612,6 +622,8 @@ readr_to_csv_convert_options <- function(na,
#' system (`SubTreeFileSystem`)
#' @param include_header Whether to write an initial header line with column names
#' @param batch_size Maximum number of rows processed at a time. Default is 1024.
+#' @param write_options read_options see [file reader options][CsvWriteOptions]
Review comment:
The downside to this approach is that we do not have much in terms of content/ docs for `CsvWriteOptions`. We direct to it from a couple of places, but maybe we need to extend the documentation, more so once we surface more options from the C++ code. Happy to open a follow-up Jira and update the `CsvWriteOptions` documentation.
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758613417
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,58 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ paste("You have supplied both \"file\" and \"sink\" arguments. Please",
+ "supply only one of them."),
+ call. = FALSE
+ )
Review comment:
Done
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r761013361
##########
File path: r/R/csv.R
##########
@@ -415,6 +415,16 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
)
}
+readr_to_csv_write_options <- function(include_header,
+ batch_size = 1024L) {
+ assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
Review comment:
It will not pass the assertion, more specifically the `finite = TRUE` part.
```
> is_integerish(Inf, n = 1)
[1] TRUE
> is_integerish(Inf, n = 1, finite = TRUE)
[1] FALSE
```
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r761075377
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,57 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ "You have supplied both \"file\" and \"sink\" arguments. Please ",
+ "supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ "You have supplied both \"col_names\" and \"include_header\" ",
+ "arguments. Please supply only one of them.",
+ call. = FALSE
+ )
+ }
+
+ # default value are considered missing by base R
+ if (missing(include_header) && !missing(col_names)) {
+ message(
+ "You have supplied a value for \"col_names\". This will overwrite ",
+ "the value for the \"include_headers\" argument.")
+ include_header <- col_names
+ }
Review comment:
In the end I got rid of the message. Done!
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r752092203
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
@jonkeane & @paleolimbot do we change the name of the `write_csv_arrow()` argument from `sink` to `file`? What about `include_header` to `col_names`? I think `sink` is a lot more than just a file path.
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758549669
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,58 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ paste("You have supplied both \"file\" and \"sink\" arguments. Please",
+ "supply only one of them."),
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ paste("You have supplied both \"col_names\" and \"include_header\"",
+ "arguments. Please supply only one of them."),
+ call. = FALSE
+ )
Review comment:
```suggestion
stop(
"You have supplied both \"col_names\" and \"include_header\" ",
"arguments. Please supply only one of them.",
call. = FALSE
)
```
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r758613107
##########
File path: r/R/csv.R
##########
@@ -623,9 +638,58 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ file = NULL,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ col_names = NULL,
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+ unsupported_passed_args <- names(list(...))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
+
+ if (!missing(file) && !missing(sink)) {
+ stop(
+ paste("You have supplied both \"file\" and \"sink\" arguments. Please",
+ "supply only one of them."),
+ call. = FALSE
+ )
+ }
+
+ if (missing(sink) && !missing(file)) {
+ sink <- file
+ }
+
+ if (!missing(col_names) && !missing(include_header)) {
+ stop(
+ paste("You have supplied both \"col_names\" and \"include_header\"",
+ "arguments. Please supply only one of them."),
+ call. = FALSE
+ )
+ }
+
+ # default value are considered missing by base R
+ if (missing(include_header) && !missing(col_names)) {
+ message(
+ paste(
+ "You have supplied a value for \"col_names\". This will overwrite",
+ "the value for the \"include_headers\" argument."))
Review comment:
Done
--
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
[GitHub] [arrow] dragosmg commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r755178189
##########
File path: r/R/csv.R
##########
@@ -624,8 +636,27 @@ readr_to_csv_convert_options <- function(na,
write_csv_arrow <- function(x,
sink,
include_header = TRUE,
- batch_size = 1024L) {
- write_options <- CsvWriteOptions$create(include_header, batch_size)
+ batch_size = 1024L,
+ write_options = NULL,
+ ...) {
+
Review comment:
Done
--
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
[GitHub] [arrow] jonkeane commented on a change in pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#discussion_r754729014
##########
File path: r/R/csv.R
##########
@@ -623,8 +632,38 @@ readr_to_csv_convert_options <- function(na,
#' @include arrow-package.R
write_csv_arrow <- function(x,
sink,
+ na = NULL,
+ append = NULL,
+ quote = NULL,
+ escape = NULL,
+ eol = NULL,
+ num_threads = NULL,
+ progress = NULL,
include_header = TRUE,
batch_size = 1024L) {
+
+ passed_args <- as.list(match.call()[-1])
+
+ arrow_write_opts <- names(formals(CsvWriteOptions$create))
+ write_args <- names(formals(write_csv_arrow))
+
+ unsupported_opts <- setdiff(
+ write_args,
+ union(arrow_write_opts, c("x", "sink"))
+ )
+
+ unsupported_passed_args <- names(unlist(passed_args[unsupported_opts]))
+
+ if (length(unsupported_passed_args)) {
+ stop(
+ "The following ",
+ ngettext(length(unsupported_passed_args), "argument is ", "arguments are "),
+ "not yet supported in Arrow: ",
+ oxford_paste(unsupported_passed_args),
+ call. = FALSE
+ )
+ }
Review comment:
Yeah `sink` is a lot more than file path, and matches our other functions. How awkward would it be to support both sink and filename? It's a bit awkward, but it would make this match readr while also maintaining our arrow patterns.
--
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
[GitHub] [arrow] dragosmg commented on pull request #11668: ARROW-13623 [R] write_csv_arrow update to follow the signature of readr::write_csv
Posted by GitBox <gi...@apache.org>.
dragosmg commented on pull request #11668:
URL: https://github.com/apache/arrow/pull/11668#issuecomment-972861463
@paleolimbot & @jonkeane I think the current state of the PR addresses your suggestions. The only thing we need to decide on is whether to keep the current names for `include_header` and `sink`. I think we should. What do you think?
--
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