Add functionality to connect to local timeseries target data #77
+799
−2
Conversation
This file contains 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
Including removing a couple of unnecessary explicit retuns
This allows for correct schema determination when timeseries data is partitioned on a date column that does not correspond to valid date task ID column.
zkamvar
reviewed
Mar 3, 2025
There was a problem hiding this comment.
Looks good to me! I think the date_col argument is a good compromise.
The only thing I would suggest is to use {gert} instead of {git2r}. There are a few advantages:
- 10x smaller than {git2r}
- {usethis} uses it on the backend, so devs will likely already have it installed
- it's more "batteries included" than {git2r} (https://docs.ropensci.org/gert/#differences-with-git2r)
|
|
||
| structure(ts_data, | ||
| class = c("target_timeseries", class(ts_data)), | ||
| ts_path = as.character(fs::path_rel(ts_path, hub_path)) |
There was a problem hiding this comment.
So glad you did this---adding the as.character() is always something that I forget to do with fs paths and it always bites me in the end.
|
|
||
| expect_error( | ||
| create_timeseries_schema(tmp_hub_path), | ||
| "No .*date.* type column found in .*time-series.*." |
There was a problem hiding this comment.
oh this is a clever solution to getting around formatting issues!
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.
This PR resolves #71
It's the first step in accessing target data and focuses on accessing local timeseries target as a first pass.
It also adds a function for creating an expected schema for the timeseries target data that is applied during read.
Validation is not tackled here nor is reading target data from the cloud.
The tests demonstrate that the function can be used to successfully read time-series files that are:
time-series.csvformat.time-seriesdirectory (including with sub-directories)time-seriesdirectoryNote that when partitioning on a column that does not match any date task IDs (e.g. date column in timeseries is called
dateand the data are partitioned on it but the equivalent task ID istarget_end_date), the date type is not automatically detectable, even if the files are parquet files. As such, I've included adate_colargument that allows us to specify that column explicitly. Eventually this should be configured via any target configuration file we decide on.Also removed a couple explicit returns to appease lintr