ARROW-13399: [R] Update dataset.Rmd vignette

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!

[thisisnic]

Various updates to dataset.Rmd including:

• separating out dense text chunks
• rephrasing based on suggestions by Grammarly to simplify phrasing
• rephrasing "we" to "you"

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13399

[thisisnic]

I've also removed a lot of the backticks around "arrow" and "dplyr" as I think they make the text harder to scan. I took a look at the tidyverse packages and the convention in their vignettes seems to be to link to any external packages the first time they're mentioned, and then subsequently just treat the package names as if they are words.

[jonkeane]

This looks like a really nice enhancement for readability and style. I have a few comments (and a number of them are "we should standardise this!"), so feel free to take them, discard them, or punt on them

[jonkeane]

I could be misremembered (or remembering the issue but not that it was resolved), but I thought there were some options that weren't compatible with the dataset version of csv reading. We don't have to list them here, but if that is true + those are documented elsewhere (like the docs for read_csv_arrow() or open_dataset()), maybe a link to that elsewhere that contains those caveats would be nice.

[thisisnic]

I have no recollection of that but I'll have a search on JIRA and see what I can find

[thisisnic]

Had a look in the docs and I found this in open_dataset():

additional arguments passed to dataset_factory() when sources is a directory path/URI or vector of file paths/URIs

Is this what you meant, as in it only works for certain values of sources, or something else?

[jonkeane]

I was thinking about https://github.com/apache/arrow/blob/master/r/R/dataset-format.R#L135-L153 bit.

Running the code in the middle of that, I get:

> # Catch any readr-style options specified with full option names that are
> # supported by read_delim_arrow() (and its wrappers) but are not yet
> # supported here
> unsup_readr_opts <- setdiff(
+ names(formals(read_delim_arrow)),
+ names(formals(readr_to_csv_parse_options))
+ )
> unsup_readr_opts
[1] "file" "schema" "col_names" "col_types" "col_select" [6] "na" "quoted_na" "skip" "parse_options" "convert_options" [11] "read_options" "as_data_frame" "timestamp_parsers"

[nealrichardson]

you may want to intersect that with names(formals(readr::read_delim)) since some of those are arrow function args

[thisisnic]

I'm confused by @nealrichardson 's comment, please can you rephrase that?

In the meantime, it looks like there are only 5 parsing options supported and more unsupported, so I feel like I'm better off explicitly listing the ones that are supported.

[nealrichardson]

Many of the args in unsup_readr_opts aren't actually from readr, they're just arguments to read_delim_arrow that aren't in readr_to_csv_parse_options. (In practice where this code is run, this doesn't matter because if you supplied as_data_frame as an argument, it will have matched that and not be in the ... passed in here.) We only care about the ones that are readr options, so:

> intersect(unsup_readr_opts, names(formals(readr::read_delim)))
[1] "file" "col_names" "col_types" "col_select" "na" [6] "quoted_na" "skip" 

[thisisnic]

Right, I get you, I think. I think that now I'm explicitly specifying the arguments that can be passed through rather than can't, I don't think I need to make any more changes here on account of the above comments?

[jonkeane]

I wonder if it might be clearer / easier to wade through if we use c("year", "month") instead of "a character vector"? That way the values are right here, it will be obvious to many R users what that is, even if they don't have "character vector" in their vocabulary.

[thisisnic]

Great suggestion, being explicit about this can only help understanding.

[thisisnic]

Done

[jonkeane]

This might be beating a dead horse, but maybe it would be good to repeat "even though they are not present in the files themselves" here too?

[thisisnic]

Not beating a dead horse at all, being explicit about these things makes it a lot easier to understand with less effort.

[thisisnic]

Done

[jonkeane]

Suggested change

	Here's an example. Suppose that you are curious about tipping behavior among the
	Here's an example: Suppose that you are curious about tipping behavior among the

Minor, and stylistic, feel free to disregard

[thisisnic]

Done

[thisisnic]

Does it? I think we should delete this sentence but it'd be good to hear other people's thoughts on this first.

[nealrichardson]

At one point this was true (the discussion at the end talked about what's not yet implemented but coming) but perhaps that's no longer accurate.

[nealrichardson]

Some final nits; LGTM thanks!