Skip to content

Replace geoarrow with sfarrow - #1116

Open
wrridgeway wants to merge 9 commits into
masterfrom
replace-geoarrow-with-sfarrow
Open

wrridgeway wants to merge 9 commits into
masterfrom
replace-geoarrow-with-sfarrow

Conversation

@wrridgeway

@wrridgeway wrridgeway commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

I did significantly less testing for this PR than #1100, but I also made much smaller changes. sfarrow is not having any issues reading our current geoparquet files and handling them the same as geoarrow did as far as I can tell. I did rewrite the parcel geoparquet for 2025 in the dev bucket, as well as the neighborhood geoparquet for 2021. I rebuilt z_de_wridgeway_location.vw_pin10_location_fill and reran my testing script and got exactly the same results as in #1110. Script is pasted below. Note, this PR will not make the changes to the parcel files #1100 did that lead to the tax district changes shown below. Those are persisted from rebuilding raw parcel data in that PR and using a different source for attributes.

vw_pin10_location_fill

The ultimate test to see if using standard, non-spatial parquet files in athena produces the same spatial results as using geo-parquets made with geoarrow is to build our location db and check the new version of vw_pin10_location_fill against the old version.

library(arrow)
library(DBI)
library(dplyr)
library(noctua)
library(purrr)

noctua_options(unload = TRUE)
AWS_ATHENA_CONN_NOCTUA <- dbConnect(noctua::athena(), rstudio_conn_tab = FALSE)

old <- dbGetQuery(conn = AWS_ATHENA_CONN_NOCTUA, "
select * from location.vw_pin10_location_fill where year = '2025'
") %>%
  arrange(pin10)

new <- dbGetQuery(conn = AWS_ATHENA_CONN_NOCTUA, "
select * from z_dev_wridgeway_location.vw_pin10_location_fill where year = '2025'
") %>%
  arrange(pin10)

diffs <- names(old)[names(old) != "pin10"] %>%
  set_names() %>%
  map(function(x) {
  old <- old %>% select(pin10, "old" = all_of(x))
  new <- new %>% select(pin10, "new" = all_of(x))
  compare <- inner_join(old, new, by = "pin10") %>%
   filter(old != new | (!is.na(old) & is.na(new)) | (!is.na(new) & is.na(old)))

  if (nrow(compare) > 0) {
    return(compare)
  } else {
    return(NULL)
  }

  })

diffs <- diffs[lengths(diffs) != 0]
> names(diffs)
 [1] "tax_municipality_num"                "tax_municipality_name"               "combined_municipality_name"         
 [4] "tax_school_elementary_district_num"  "tax_school_elementary_district_name" "tax_school_secondary_district_num"  
 [7] "tax_school_secondary_district_name"  "tax_school_unified_district_num"     "tax_school_unified_district_name"   
[10] "tax_community_college_district_num"  "tax_community_college_district_name" "tax_fire_protection_district_num"   
[13] "tax_fire_protection_district_name"   "tax_library_district_num"            "tax_library_district_name"          
[16] "tax_park_district_num"               "tax_park_district_name"              "tax_sanitation_district_num"        
[19] "tax_sanitation_district_name"        "tax_special_service_area_num"        "tax_special_service_area_name"      
[22] "tax_tif_district_num"                "tax_tif_district_name"               "tax_data_year"

Comment on lines +9 to +18
# rJava's LD_LIBRARY_PATH from .Renviron isn't picked up by the dynamic
# linker in time, so preload libjvm.so by absolute path first.
dyn.load("/usr/lib/jvm/java-21-openjdk-amd64/lib/server/libjvm.so")
# tabulizer/RJavaTools reflects into JDK internals (e.g. java.util
# iterators), which Java 16+'s module system denies by default (JEP 396).
# Must be set before the JVM starts, i.e. before library(rJava).
options(java.parameters = c(
"--add-opens=java.base/java.util=ALL-UNNAMED",
"--add-opens=java.base/java.lang=ALL-UNNAMED"
))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're iffy about this, but I tried tabulapdf and ran into the same exact issue. The good news is that this is one small file that we should never need to rebuild, not something we have to do on a yearly basis. If you'd like, we could open a new PR to try and figure out some other solution that moves away from tabulizer and tabulapdf. I'd prefer not to get bogged down in fixing this problem in this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I opened #1119 to track the effort to simplify this. I'll dig into it in November if I can find the time.

esri_chicago_geojson <- here(footprint_tmp_dir, "chicago-2008.geojson")
esri_chicago_remote_raw <- file.path(
input_bucket, "esri", "chicago-2008.parquet"
input_bucket, "esri", "chicago-2008.geojson"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files were incorrectly specified.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm am leaving the big changes from #1100 for another PR.

Comment on lines +24 to 29
parcels <- sfarrow::read_sf_dataset(arrow::open_dataset(
paste0("s3://ccao-data-warehouse-us-east-1/spatial/parcel/year=", year)
))
)) %>%
select(-geometry) %>%
rename(geometry = geometry_3435)

@wrridgeway wrridgeway Oct 29, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it's from updating sf, but this script throws a ton of errors when we're dealing with two geometry columns. Since it immediately sets the parcels object to epsg:3435 for processing on line 30 (gone now) I'm just getting rid of the 4326 column right off the bat here. The raw parcel geoparquets are all saved with geometry_3435 as their active geometry column, so they ingest with a crs of epsg:3435 and renaming geometry_3435 means we don't have to edit all the spatial manipulation below.

@wrridgeway
wrridgeway marked this pull request as ready for review October 29, 2026 17:00
@wrridgeway
wrridgeway requested a review from a team as a code owner October 29, 2026 17:00
Comment thread etl/renv.lock

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We update a number of packages for compatibility with R 4.6.1.

@jeancochrane jeancochrane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your persistence! I like that this change is much simpler than #1100, even though sfarrow is not actively maintained.

library(arrow)
library(ccao)
library(DBI)
library(geoarrow)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question, non-blocking] Do you have any idea why this script doesn't write out files like the rest of the ETL raw scripts do? I assume it's not causing problems but I'm curious.

Comment on lines +9 to +18
# rJava's LD_LIBRARY_PATH from .Renviron isn't picked up by the dynamic
# linker in time, so preload libjvm.so by absolute path first.
dyn.load("/usr/lib/jvm/java-21-openjdk-amd64/lib/server/libjvm.so")
# tabulizer/RJavaTools reflects into JDK internals (e.g. java.util
# iterators), which Java 16+'s module system denies by default (JEP 396).
# Must be set before the JVM starts, i.e. before library(rJava).
options(java.parameters = c(
"--add-opens=java.base/java.util=ALL-UNNAMED",
"--add-opens=java.base/java.lang=ALL-UNNAMED"
))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I opened #1119 to track the effort to simplify this. I'll dig into it in November if I can find the time.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants