Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates geospatial ingestion/transformation away from in-memory geopandas/pandas to a PostGIS-native pipeline: ingestion streams data into PostGIS via ogr2ogr (GDAL), and transformations are built once as parameterized SQL (SQLAlchemy Core) for both preview and process paths. It also bumps the repo’s Python pin to 3.13 and updates the Airflow image build/dev tooling to a Debian Trixie-based setup that can ship newer GDAL.
Changes:
- Replace geopandas-based ingestion with
ogr2ogrsubprocess calls (file/URL/FTP/DB/OGC services) and update unit tests accordingly. - Introduce SQL-native transformation builder (
build_transformation_select) plus process (CTAS) and preview (bounded rows + GeoJSON) paths. - Upgrade Python pin to 3.13 and update Airflow tooling (local base image build + GDAL install + compose args + workflow).
Reviewed changes
Copilot reviewed 39 out of 42 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrightconfig.json | Update static-analysis Python version to 3.13. |
| pyproject.toml | Update workspace requires-python to 3.13. |
| Makefile | Add Airflow base-image build target and related variables. |
| libs/data_manipulation/tests/test_transformation.py | Replace geopandas transformation tests with SQL compilation/assertion tests. |
| libs/data_manipulation/tests/test_ingestion.py | Replace geopandas ingestion tests with ogr2ogr command-building tests. |
| libs/data_manipulation/tests/test_column_actions.py | Refocus tests on SQL-level filtering/selection helpers. |
| libs/data_manipulation/src/data_manipulation/utils.py | Remove GeoDataFrame detection helper (geopandas dependency removal). |
| libs/data_manipulation/src/data_manipulation/transformation/transform.py | Remove in-memory transformation pipeline. |
| libs/data_manipulation/src/data_manipulation/transformation/transform_projection.py | Remove in-memory CRS relabel helper. |
| libs/data_manipulation/src/data_manipulation/transformation/transform_geom_point.py | Remove in-memory point-geometry builder. |
| libs/data_manipulation/src/data_manipulation/transformation/transform_encoding.py | Remove in-memory encoding transform. |
| libs/data_manipulation/src/data_manipulation/transformation/transform_columns.py | Remove in-memory rename/cast helpers (superseded by SQL). |
| libs/data_manipulation/src/data_manipulation/transformation/sql_transform.py | Add canonical SQL-native transformation builder + preview + CTAS process path. |
| libs/data_manipulation/src/data_manipulation/transformation/filter_sql.py | Factor out a reusable build_filter_clause helper and reuse it. |
| libs/data_manipulation/src/data_manipulation/ingestion.py | Implement ogr2ogr-based ingestion for all sources and supporting helpers. |
| libs/data_manipulation/src/data_manipulation/constants.py | Add default OGC SRS constant to avoid SRID 0 ingestion. |
| libs/data_manipulation/src/data_manipulation/init.py | Update public exports to new ingestion/SQL-transform APIs. |
| libs/data_manipulation/README.md | Document the new ogr2ogr + SQL-native design and GDAL requirements. |
| libs/data_manipulation/pyproject.toml | Bump Python pin; drop geopandas/pyarrow; add pyproj. |
| libs/data_manipulation/.python-version | Bump to Python 3.13. |
| docker/Dockerfile.airflow | Switch to Trixie-based Airflow base image; install newer GDAL via micromamba. |
| docker/compose.airflow.yaml | Wire in AIRFLOW_BASE_IMAGE build arg and bump Airflow version defaults. |
| docker/airflow-base/scripts/docker/keys/postgres.asc | Add upstream key material for building the Trixie Airflow base image. |
| docker/airflow-base/scripts/docker/keys/microsoft.asc | Add upstream key material for building the Trixie Airflow base image. |
| docker/airflow-base/scripts/docker/keys/mariadb.asc | Add upstream key material for building the Trixie Airflow base image. |
| docker/airflow-base/README.md | Document local build of Trixie Airflow base image. |
| ARCHITECTURE.md | Update architecture notes to reflect server-side SQL transformations and ogr2ogr ingestion. |
| apps/elt/pyproject.toml | Bump Python pin; update Airflow/provider versions. |
| apps/elt/dags/utils.py | Remove pandas dependency and add raw-cursor helper to return dict rows. |
| apps/elt/dags/task_groups/transformation.py | Replace read/transform/write with DB-native transform_staging_to_final CTAS. |
| apps/elt/dags/task_groups/ingestion.py | Add passing decrypted Basic Auth to OGC ingestion (GDAL config). |
| apps/elt/.python-version | Bump to Python 3.13. |
| apps/backend/src/api/routes/ingestion/staging.py | Replace geopandas preview logic with read_transformed_preview + SRID detection helper. |
| apps/backend/pyproject.toml | Bump Python pin; align Airflow client version; drop chardet. |
| apps/backend/Dockerfile | Bump base Python image to 3.13 and update uv python selection. |
| apps/backend/.python-version | Bump to Python 3.13. |
| .python-version | Bump repo default to Python 3.13. |
| .github/workflows/build-airflow-trixie-image.yml | Add workflow to build/push the Trixie Airflow base image. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+200
to
+201
| command_string = " ".join(command) | ||
| logger.debug(f"Running command: {command_string}") |
Comment on lines
+324
to
+326
| command_string = " ".join(command) | ||
| logger.debug(f"Running command: {command_string}") | ||
| # -------- |
Comment on lines
+398
to
+400
| command_string = " ".join(command) | ||
| logger.debug(f"Running command: {command_string}") | ||
|
|
Comment on lines
+263
to
+271
| start = time.time() | ||
|
|
||
| ingest_file_with_ogr2ogr(str(temp_file_path), table_name, engine, schema) | ||
|
|
||
| # Calculate the end time and time taken | ||
| end = time.time() | ||
| length = end - start | ||
|
|
||
| print("It took", length, "seconds.") |
Comment on lines
+6
to
+9
| AIRFLOW_VERSION ?= 3.2.2 | ||
| AIRFLOW_PYTHON_VERSION ?= 3.13.5 | ||
| AIRFLOW_BASE_IMAGE ?= georchestra/airflow-base:$(AIRFLOW_VERSION)-trixie | ||
| export AIRFLOW_VERSION |
| docker build \ | ||
| --build-arg BASE_IMAGE=debian:trixie-slim \ | ||
| --build-arg AIRFLOW_VERSION=3.2.2 \ | ||
| --build-arg AIRFLOW_PYTHON_VERSION=3.14.0 \ |
Comment on lines
+34
to
+35
| - name: Push airflow | ||
| run: docker push georchestra/airflow-base:latest |
Comment on lines
+37
to
+41
| - name: Push release | ||
| if: github.ref_type == 'tag' | ||
| run: | | ||
| docker tag georchestra/airflow-base:latest georchestra/airflow-base:${{ steps.version.outputs.VERSION }} | ||
| docker push georchestra/airflow-base:${{ steps.version.outputs.VERSION }} |
Comment on lines
+32
to
+40
| url = engine.url | ||
| pg_conn_parts = [ | ||
| f"host={url.host}", | ||
| f"port={url.port or 5432}", | ||
| f"dbname={url.database}", | ||
| f"user={url.username}", | ||
| f"password={url.password}", | ||
| ] | ||
| return "PG:" + " ".join(part for part in pg_conn_parts if part.split("=", 1)[1]) |
…r PostGIS ingestion
… debian trixie and gdal
The pandas removal dropped the dependency from apps/elt/uv.lock but left process-dag-generator.py calling PostgresHook.get_pandas_df(). Since this module runs at scheduler parse time, the ImportError would prevent every scheduled ingestion DAG from being generated. get_records_as_dicts() was already added to utils.py as the replacement but was never wired up. Call it, and drop the now-unused get_datafeeder_pg_hook import. The test stubbed get_pandas_df, so it passed against the broken path; stub the new helper instead so it exercises the real code.
Three call sites joined the full ogr2ogr argv and logged it at DEBUG, directly above the comment warning that the command must never be logged. The argv embeds the PG: connection string (database password) and, for OGC services, GDAL_HTTP_USERPWD. Airflow task logs are readable from the web UI, so enabling DEBUG exposed these secrets. Remove the three logger.debug calls. The surrounding logger.info lines already identify the source and target without any credential. Add regression tests asserting no password reaches the log records for the file, database-to-database and OGC paths.
Library code wrote ingestion timing to stdout with print(), bypassing the configured logging and polluting worker output. Drop it along with the time.time() scaffolding and the now-unused time import.
The airflow profile services build FROM the locally-built base image, which is never published to a registry, so `make up` failed on a fresh checkout. The target is already guarded by a `docker images -q` check, so it is a no-op once the image exists.
response.content materialised the entire response body in memory before writing it to the temp file, so a multi-GB source was fully resident in RAM. This raised peak memory above the chunked geopandas reader it replaced, contradicting the goal of handing files straight to GDAL. Stream with stream=True and iter_content into the temp file. Headers are still read before the body is consumed, so Content-Disposition filename extraction is unchanged. iter_content is used rather than copyfileobj(response.raw) because it applies Content-Encoding, whereas raw would write compressed bytes. Add tests covering the URL path, which had no coverage: one asserts the body is streamed (the fake response raises on .content), the other that Content-Disposition still names the temp file.
Zipped shapefiles were passed to ogr2ogr as a plain path, which GDAL
cannot open: ogr.Open("x.zip") fails with "not recognized as being in a
supported file format". Since main relied on fiona (which applies zip://
itself), handing the raw path to the ogr2ogr binary regressed a common
input format.
Resolve ZIPs to /vsizip/<archive>, appending the subdirectory when the
dataset is not at the archive root — /vsizip/<archive> alone fails for a
nested layout.
Reject archives holding more than one dataset. ogr2ogr -nln writes every
layer into the same table, so with -overwrite each layer replaced the
previous one and only the last survived, with a zero exit code and
nothing in the logs. The error names the datasets found so the user knows
which to extract. Shapefile sidecars (.shx/.dbf/.prj/...) are grouped
with their .shp so they don't count as separate datasets.
Verified against GDAL 3.12 for flat and nested archives; GeoPackage is
unaffected.
-a_srs was applied to every OGC ingestion. It relabels the CRS without reprojecting, which is correct for OAPIF (GeoJSON is WGS84 lon/lat per RFC 7946) but wrong for WFS: a WFS serves whatever srsName was negotiated, commonly a projected CRS such as EPSG:2154. Forcing 4326 on such a service tagged metric coordinates as degrees, so the downstream ST_Transform placed the data far from its real location — with no error and a zero exit code. Restrict -a_srs to the ogcFeatures protocol and let GDAL keep the SRS the WFS advertises. The WFS test asserted the old behaviour, so invert it and add the matching OAPIF case.
CREATE TABLE AS copies data and column types but no indexes, so the GiST index that GeoPandas' to_postgis used to create was lost. Final tables are the ones published in GeoServer, so every bbox query (WMS/WFS) degraded to a sequential scan — verified on 200k rows, where the planner switches from Seq Scan to Bitmap Heap Scan once the index exists. Recreate it as idx_<table>_<geom_col>, the name to_postgis used, keeping the behaviour iso-functional. This is also the name that POSTGIS_TABLE_NAME_MAX_LENGTH (54) is sized for: at the maximum table length the index name is exactly 63 chars, PostgreSQL's identifier cap. Only geographic results get an index; tq.geom_column is None for tabular data.
main declared chardet in data_manipulation for the geopandas encoding detection, which this branch replaces with ogr2ogr. Nothing imports it anymore, so remove the declaration. The rebase also left uv.lock referencing chardet from data-manipulation without the matching [[package]] entry, which made `uv lock` fail to parse the file. Drop those two stale references and relock.
Two ways an ogr2ogr failure went unreported:
- text=True decoded stderr as strict UTF-8, but ogr2ogr echoes the
offending record when it rejects non-UTF-8 input. Decoding then raised
UnicodeDecodeError and hid GDAL's actual message behind a Python
traceback. Decode with errors="replace".
- ogr2ogr exits 0 even after aborting a layer translation ("ERROR 1: Non
UTF-8 content found ... Terminating translation prematurely"), so
check=True reported success while no table had been created. Scan
stderr for GDAL error lines and raise. The regex is anchored at the
start of a line so a path containing "error" does not match.
Also chain CalledProcessError with `from exc`, which was missing.
…t .cpg The move to ogr2ogr dropped the chardet-based detection that main had just hardened (PR #107). GDAL covers most of it natively — it reads the .cpg sidecar and assumes UTF-8 otherwise — but a shapefile shipped without a .cpg in a Western European codepage then makes ogr2ogr abort with "Non UTF-8 content found", writing no table at all. Detect that case only: sample the .dbf (directly or inside the ZIP, as main did) and pass --config SHAPE_ENCODING. When a .cpg exists, or the content is ASCII/UTF-8, defer to GDAL and add nothing. Two details found while testing against a real PostGIS: - chardet classifies a whole .dbf as binary because the fixed-width header drowns out the text, and the trailing 0x1A EOF marker alone is enough to make it give up. Feed it the record section with 0x1A stripped. - on such short samples chardet cannot separate the Latin codepages and returns cp1250 for Western text, which decodes "ê" as "ę". Collapse those onto CP1252, what shapefiles without a .cpg overwhelmingly use here. chardet returns as a data_manipulation dependency since it is imported again. Verified end to end: shapefile and zipped shapefile, with and without .cpg, plus GeoJSON, all yield "Café"/"Forêt" in PostGIS.
This file contains hidden or 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
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.
No description provided.