Repository navigation
Replace geoarrow with sfarrow in geometry ingest - #110
Conversation
| sfarrow::read_sf_dataset() %>% | ||
| # Fill missing townships, prefering the newest available | ||
| arrange(pin10, year) %>% | ||
| fill(town_code, .direction = "updown", .by = pin10) %>% |
There was a problem hiding this comment.
While taking a look at this code, I noticed that the .by param was only added to tidyr::fill() in version 1.3.2, which is currently the latest version in the version list. Switching to the more-verbose group_by() makes this code compatible with a wider range of tidyr versions, which I think is good practice for a package like this that might be installed alongside a variety of different versions of its dependencies.
| filter(year >= 2006 & year <= max_year) %>% | ||
| select(year, x = lon, y = lat, pin10, town_code, geometry) %>% | ||
| geoarrow_collect_sf() %>% | ||
| sfarrow::read_sf_dataset() %>% |
There was a problem hiding this comment.
Note that this read_sf_dataset() call raises a warning, which I think is ultimately benign:
In arrow_to_sf(tbl, geo) :
Primary geometry column not found, using next available.
I think what's happening here is that geometry_3435 is set as the primary geometry column in the spatial.parcel parquet file metadata, but we're not selecting it in select(). Since it's missing, read_sf_dataset() warns that it can't find the primary geometry column, and it falls back to using the other geometry column that is present instead (i.e. geometry).
Quick script to confirm that geometry_3435 is the primary geometry column:
library(arrow)
ds <- open_dataset("s3://ccao-data-warehouse-us-east-1/spatial/parcel")
geo <- jsonlite::fromJSON(ds$metadata$geo)
print(geo$primary_column)[1] "geometry_3435"
I thought about two ways of resolving the warning, neither of which seem exactly right to me:
- Suppress it with
suppressWarnings(): This would prevent the warning from printing to the console, but might suppress other, more important warnings in the future, so I decided against it. - Select
geometry_3435in theselect()call, and then drop it after settinggeometryas the geometry column: This would resolve the warning without suppressing other warnings, but it would roughly double the memory use of the script, since we would need to load bothgeometryandgeometry_3435into memory. This script already bumps up against the limits of the available memory on our biggest machines, so I don't think doubling memory use is a good idea.
Ultimately I think it's just best to leave the harmless warning as-is, but I'm open to pushback.
There was a problem hiding this comment.
You actually saw me fighting with this a bit in my first PR to replace geoarrow in the etl pipeline (scripts that assume geometry_3435 vs geometry will be the default geo column). It's really frustrating that our parcel spatial data is set to use geometry_3435 as the primary geometry column and no other spatial data is. I'd say we should fix this, but there would likely be a lot of subtle downstream effects that are hard to detect. I think it's fine to leave the warning, especially since sfarrow natively moves onto the only other remaining geo column.
There was a problem hiding this comment.
I'm definitely ok with leaving as is!
wrridgeway
left a comment
There was a problem hiding this comment.
Thanks Jean! Glad my hunch was right (it seems?) and those diffs were just from the way I had to run this script locally on my laptop.
| filter(year >= 2006 & year <= max_year) %>% | ||
| select(year, x = lon, y = lat, pin10, town_code, geometry) %>% | ||
| geoarrow_collect_sf() %>% | ||
| sfarrow::read_sf_dataset() %>% |
There was a problem hiding this comment.
You actually saw me fighting with this a bit in my first PR to replace geoarrow in the etl pipeline (scripts that assume geometry_3435 vs geometry will be the default geo column). It's really frustrating that our parcel spatial data is set to use geometry_3435 as the primary geometry column and no other spatial data is. I'd say we should fix this, but there would likely be a lot of subtle downstream effects that are hard to detect. I think it's fine to leave the warning, especially since sfarrow natively moves onto the only other remaining geo column.
kyrasturgill
left a comment
There was a problem hiding this comment.
Thanks for handling this! Excited to leave geoarrow behind us.
Background
This PR fixes the geometry ingest by switching from
geoarrowtosfarrow. We do this because the version ofgeoarrowthat we rely on (0.1.0) is very outdated and is no longer compatible with R 4.6. This change mirrors an identical change we made indata-architecturerecently: ccao-data/data-architecture#1116The new geometry output is in
s3://z-dev-ccao-data-ptaxsim-us-east-1/pin_geometry_raw/part-0.parquet. Once we're ready to construct the 2025 database, we can copy this file into the prod bucket.Closes #108.
Testing
I had two goals in testing this change:
sfarrowproduces identical geometry columns1. Confirm the script resolves unexpectedly-changed year ranges
Save this script to
data-raw/pin/test_geometry_ranges.R:Expand to view the QC script
Run it on all the blocks we found to have altered ranges:
Output:
2. Confirm that
sfarrowproduces identical geometry columnsSave this script to
data-raw/pin/qc_geometry_vs_2024.R:Expand to view the QC script
Run the script:
Output: