fix: 🐛 use duckplyr to read Parquet as DuckDB - #348
Conversation
|
I assume that the failing GHA is a fastreg thing and not on my end, but let me know if I should look into it. |
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Nice! Just some comments
| duckplyr::as_duckdb_tibble() |> | ||
| duckplyr::as_tbl() |
There was a problem hiding this comment.
Why are both these needed? And doesn't that negate the thrifty?
There was a problem hiding this comment.
I’ll have to look into that. It mirrors the existing osdc testing pipeline when we set up the simulated data, so I assumed it was good. I didn’t dive deep into the technical details, this was just what “worked”, so I ran with it.
The prudence choice is also quite important - I figured thrifty is going to strike the best balance between memory safety and practical usefulness for quick exploratory summary statistics etc, but it’s definitely up for discussion.
There was a problem hiding this comment.
I looked into it, and you're right: duckplyr::as_tbl() strips the duckplyr_df class and removes the prudence guard rail feature.
I think the right way to go about it here in fastreg is to NOT pipe it through as_tbl(). The user can always pipe the output from fastreg through as_tbl() themselves if they need to.
Users of the osdc package would be one example of people needing to pipe it through as_tbl() (see steno-aarhus/osdc#578), but that pipeline is much more complex than most users'.
| test_that("read_parquet_dataset() errors with incompatible schemas", { | ||
| # Create a bef file where numeric columns are changed to character, so | ||
| # the schema is incompatible with the other bef files. | ||
| incompatible_data <- bef_list$data[[1]] |> | ||
| dplyr::mutate(dplyr::across(where(is.numeric), as.character)) | ||
|
|
||
| incompatible_sas_path <- fs::path_temp( | ||
| "sas_schema_incompatible/bef2099.sas7bdat" | ||
| ) | ||
| write_to_sas(incompatible_data, incompatible_sas_path) | ||
| sas_incompatible <- c( | ||
| sas_paths$output_path, | ||
| incompatible_sas_path | ||
| ) | ||
|
|
||
| incompatible_output <- fs::path_temp("incompatible") | ||
| # Convert files. | ||
| purrr::walk(sas_incompatible, \(path) { | ||
| convert(path, incompatible_output) | ||
| }) | ||
|
|
||
| expect_error(read_parquet_dataset(incompatible_output), "incompatible") | ||
| }) | ||
|
|
There was a problem hiding this comment.
Why remove this? The point was to ensure that it correctly fails if there is an incompatible schema.
There was a problem hiding this comment.
The duckdb reader is inherently much more robust to type mismatches than Arrow’s. There doesn’t appear to be a way to make duckplyr::read_parquet_duckdb() fail that test, so I think it has to be removed.
There was a problem hiding this comment.
As I see it, DuckDB's behavior is preferable to Arrow's - especially from a reproducibility POV:
With arrow's Parquet reader, you would receive an error and then have to dig through all the Parquet files to find the file(s) with the diverging schema and convert them. Here, you're forced to set up a separate workflow to do this.
With duckplyr's reader, the data is read without issue, and you simply pipe it through a mutate() call to force the expected type and make it explicit. Here, you don't need to do anything outside of your standard data loading pipeline.
|
I've updated the PR to keep the outputs as |
2 participants
Description
Changes
read_parquet_dataset()andread_parquet_file()to useduckplyr::read_parquet_duckdb()instead ofarrow::open_dataset()/arrow::read_parquet()This provides more robust support for duckplyr verbs and avoids errors/bugs as mentioned in #346.
Closes #346
The change to the functions is small, and only a few edits to test-read.R and test-use.R were necessary. The only substantial change to the tests is the removal of the test for incompatible schemas in test-read.R, which was necessary because duckplyr::read_parquet_duckdb handles this very differently from how arrow::open_dataset() does: if a variable's type differs between Parquet files, it is silently/automatically converted/promoted to a compatible type if at all possible and no error is thrown. I could not find a an option/flag to change this behavior.
I'd argue that, as a user this is actually the preferable behavior, since it doesn't require debugging/re-running the whole conversion, but just a single
mutate()call to force the desired type.I've kept the use of
arrow::open_datasetin test-convert.R untouched for two reasons:duckplyr::read_parquet_duckdbleads to some minor discrepancies in types and attributes in the data (described in the linked issue) that causeexpect_identical()to fail vs.haven-read tables, so it's a bit more messy to run those tests with duckplyr in the pipeline.Needs a quick/thorough review.
Checklist
just run-all