GH-38771: [R][Documentation] Document add_filename on open_dataset help page - #51392
Conversation
|
|
github-actions
Bot
added
Component: R
awaiting committer review
There was a problem hiding this comment.
🟡 Changes recommended
Documentation claims about partition inference, dataset applicability, and grouping behavior need correction before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Documents add_filename() usage and limitations across the R dataset documentation.
Changes:
- Adds usage guidance and examples.
- Clarifies helper constraints.
- Updates generated Rd documentation.
File summaries
| File | Summary |
|---|---|
r/vignettes/dataset.Rmd |
Adds filename guidance to the Dataset vignette. |
r/R/dplyr-funcs-augmented.R |
Clarifies add_filename() usage. |
r/R/dataset.R |
Adds open_dataset() documentation guidance. |
r/man/open_dataset.Rd |
Updates generated help documentation. |
r/man/add_filename.Rd |
Updates generated function documentation. |
Review details
Files not reviewed (2)
- r/man/add_filename.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
Suppressed comments (2)
r/R/dataset.R:81
- Opening a partition subdirectory does not prevent inference of nested partition segments (for example,
month=1remains inferable underyear=2015); only partition values encoded above the supplied root are lost. Please narrow this claim (the vignette repeats it) so users do not thinkadd_filename()is required to recover every partition column.
#' This is useful, for example, when you have opened a subdirectory of a
#' partitioned dataset directly (so the partition columns are not inferred) and
#' want to recover the partition values from the path. `add_filename()` can only
r/R/dataset.R:84
- This blanket restriction is contradicted by the existing test at
r/tests/testthat/test-dataset.R:1572-1576:mutate(file = add_filename()) |> group_by(file) |> summarise(...)succeeds withoutcompute()orcollect(). Please narrow the warning to downstream expressions that require materialization (or document the supported grouping case), otherwise the new help text tells users that a working query is invalid.
#' referenced by later steps of the same query until you call `compute()` or
#' `collect()`. See [add_filename()] for details.
- Files reviewed: 3/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The only final comment is a minor request to acknowledge the error-message change; the reviewed changes are otherwise low-risk documentation updates.
Review effort: Lite
Findings: None
Resolved since last review (1)
Files not reviewed (2)
- r/man/add_filename.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
| #' `group_by()` steps of the same query. However, it can't be used in | ||
| #' `filter()`, and some functions (such as `substr()`) are not supported on it. | ||
| #' In these cases, call \code{\link[dplyr:compute]{compute()}} or | ||
| #' \code{\link[dplyr:collect]{collect()}} first. `add_filename()` must also be |
There was a problem hiding this comment.
| #' \code{\link[dplyr:collect]{collect()}} first. `add_filename()` must also be | |
| #' \code{\link[dplyr:collect]{collect()}} first. [add_filename()] must also be |
Do we want to do this so that the link exists? Also I can never remember if we also want or need the code fencing for a link like this, it would be nice for it to be formatted as code, but that might be automatic?
There was a problem hiding this comment.
Good point, have updated
| This is handy when you have opened a single partition directory directly (for | ||
| example, `open_dataset("nyc-taxi/year=2015")`), since in that case the partition | ||
| columns above that directory (here, `year`) are not inferred and you may want to | ||
| recover them from the path. See `?add_filename` for details and limitations. |
There was a problem hiding this comment.
Same thing here? Can we have roxygen "just" link this to the thing?
github-actions
Bot
added
awaiting changes
github-actions
Bot
added
awaiting change review
github-actions
Bot
added
awaiting changes
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Correct the misleading shared diagnostic and avoid promising an absolute source path in the vignette.
Review effort: Lite
Findings: None
Files not reviewed (2)
- r/man/add_filename.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
Thanks for this! Sorry it took me so long to come back to it
github-actions
Bot
added
awaiting merge
Rationale for this change
add_filename()not documented clearlyWhat changes are included in this PR?
Document it better
Are these changes tested?
No
Are there any user-facing changes?
No just docs
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: