GH-33708: [R] read_csv_arrow()'s timestamp_parsers parameter is a bit light on documentation and doesn't appear to do anything - #51166
Open
thisisnic wants to merge 1 commit into
Conversation
|
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the stated issue/PR rationale, add appropriate warnings for a previously silent override, and include test coverage for the updated behavior.
Pull request overview
Improves the R CSV reader/dataset UX around timestamp_parsers by clarifying its semantics, surfacing a previously silent override case, and adding tests to lock in the intended behavior.
Changes:
- Expanded
timestamp_parsersdocumentation (replacement of ISO-8601 default, inference fallthrough-to-string behavior, and ISO-8601 fallback example). - Added warnings when
timestamp_parsersis supplied alongsideconvert_options(reader + dataset paths). - Added/updated tests (including unskipping the dataset case) to cover inference, ISO-8601 fallback, and the new warning.
File summaries
| File | Description |
|---|---|
| r/R/csv.R | Updates roxygen docs for timestamp_parsers/convert_options, adds example, and warns when convert_options overrides timestamp_parsers in the reader path. |
| r/R/dataset-format.R | Warns when timestamp_parsers is provided but ignored due to explicit convert_options in the dataset/file-format path. |
| r/tests/testthat/test-csv.R | Adds tests covering inference behavior, ISO-8601 fallback behavior, and the new warning for read_csv_arrow(). |
| r/tests/testthat/test-dataset-csv.R | Unskips and updates dataset test coverage for timestamp_parsers, plus warning coverage when convert_options is supplied. |
| r/man/read_delim_arrow.Rd | Regenerates man page text to reflect updated docs and example. |
| r/man/open_delim_dataset.Rd | Regenerates man page text to reflect updated docs. |
| r/man/CsvReadOptions.Rd | Updates timestamp_parsers documentation to reflect replacement semantics and fallback note. |
| r/man/csv_convert_options.Rd | Regenerates man page text to reflect updated timestamp_parsers documentation. |
Review details
Files not reviewed (4)
- r/man/CsvReadOptions.Rd: Generated file
- r/man/csv_convert_options.Rd: Generated file
- r/man/open_delim_dataset.Rd: Generated file
- r/man/read_delim_arrow.Rd: Generated file
- Files reviewed: 4/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
timestamp_parsersworks, but the docs didn't say that supplying parsers replaces the default ISO-8601 parser, or that a non-matching parser silently falls through to a string column during type inference. Passingconvert_optionsalongsidetimestamp_parsersalso silently discarded the latter.What changes are included in this PR?
Rewrites the
timestamp_parsersdocumentation and adds an example showing how to keep ISO-8601 as a fallback. Adds a warning whentimestamp_parsersis ignored becauseconvert_optionswas supplied, in both the reader and dataset paths. Unskips the dataset test for this option.Are these changes tested?
Yes, new tests for the inference path, the ISO-8601 fallback, and the warning.
Are there any user-facing changes?
Yes, a new warning when
timestamp_parsersis passed together withconvert_options.read_csv_arrow()'stimestamp_parsersparameter is a bit light on documentation and doesn't appear to do anything #33708