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.
| 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.
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