fix(go): finalize geoarrow.wkb CRS on Schema() before first Next() - #412
Draft
jatorre wants to merge 7 commits into
Draft
fix(go): finalize geoarrow.wkb CRS on Schema() before first Next()#412jatorre wants to merge 7 commits into
jatorre wants to merge 7 commits into
Conversation
Expose geospatialAsArrow support (SPARK-54232) as an opt-in ADBC connection option. When set to "true", geometry/geography columns arrive as Struct<srid: Int32, wkb: Binary> instead of EWKT strings. This depends on databricks/databricks-sql-go#328 which adds the WithArrowNativeGeospatial() ConnOption to the underlying Go SQL driver. Usage via adbc_connect (e.g. from DuckDB adbc_scanner): adbc_connect({ 'driver': 'libadbc_driver_databricks.dylib', 'databricks.server_hostname': '...', 'databricks.arrow.native_geospatial': 'true' })
When databricks.arrow.native_geospatial is enabled, the driver now converts Struct<srid: Int32, wkb: Binary> columns to flat Binary columns with ARROW:extension:name=geoarrow.wkb metadata. This enables downstream consumers (e.g. DuckDB adbc_scanner) to automatically map geometry columns to native GEOMETRY types without any explicit ST_GeomFromWKB conversion. Pipeline: Databricks -> Struct<srid,wkb> -> geoarrow.wkb -> native GEOMETRY Benchmarks vs baseline (ST_AsBinary + ST_GeomFromWKB): 100k points: 2.05x faster (31k rows/sec vs 15k rows/sec) 10k polygons: 1.31x faster (4.5k rows/sec vs 3.4k rows/sec)
Defer schema transformation to the first Next() call so the SRID can be read from the first non-null row of each geometry column. The SRID is encoded as PROJJSON CRS in ARROW:extension:metadata, e.g. EPSG:4326 or EPSG:3857. This ensures CRS information propagates correctly to downstream consumers (DuckDB, pandas, polars, GDAL). Split transformSchemaForGeoArrow into: - detectGeometryColumns: finds geometry struct column indices (called in constructor) - buildGeoArrowSchema: builds geoarrow schema with CRS from first batch (called lazily) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The schema must be available before the first Next() call since consumers like adbc_scanner read it upfront to create table columns. Build the geoarrow.wkb schema eagerly with empty CRS metadata in the constructor, then enrich it with the actual SRID from the first record batch during the first Next() call. Verified: DuckDB now correctly recognizes geometry columns as native GEOMETRY type via the geoarrow.wkb extension metadata. Benchmark results (Databricks → DuckDB): - 100k points: 7x faster than ST_AsBinary baseline - 10k polygons: 3.6x faster than ST_AsBinary baseline Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>
The current executeIngest loops over each row and issues a separate
parameterized INSERT against the SQL warehouse — measured at ~1.5 s/row
on a serverless Databricks SQL warehouse, which makes adbc_insert
impractical above ~few hundred rows. When the new
databricks.bulk.volume_path option is set, executeIngest instead
streams Arrow batches as Parquet to a Volume and issues one COPY INTO
per batch. RSS stays bounded to one batch (~37 MB at 100 K rows in a
side-by-side bench), throughput jumps from ~0.7 rows/s to ~6 K rows/s
at 100 K rows.
For geometry columns annotated as geoarrow.wkb on the Arrow schema:
- The Databricks DDL emitter (new databricksTypeForField) maps the
field to GEOMETRY(<srid>) using the EPSG code from the extension's
CRS metadata, so the destination column is a typed geometry rather
than the BINARY default. Bare GEOMETRY without an SRID modifier is
rejected on most Databricks SQL runtimes.
- The staged Parquet schema strips the extension metadata so the file
carries plain BINARY columns; the COPY INTO transform projects each
geometry column through ST_GEOMFROMWKB(<col>, <srid_literal>) so the
source binary is rebuilt as typed GEOMETRY(srid) on insert. The
SRID has to be a SQL literal — passing it as a column from the
Parquet file produces an untyped GEOMETRY whose schema can't be
merged with the destination column.
Plumbing: the connection now carries serverHostname / accessToken /
bulkVolumePath copied from databaseImpl, used by the Files API
helpers (uploadToVolume / deleteFromVolume) to PUT/DELETE staged
Parquet files via /api/2.0/fs/files{path}.
The legacy per-row INSERT path is kept as the fallback when
databricks.bulk.volume_path is unset, so this change is purely
additive and doesn't break callers without Volume access.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The geoarrow.wkb extension metadata's CRS was being set on the first
record batch via handleGeoRecord, but ADBC consumers like DuckDB's
adbc_scanner read Schema() once at construction time to bind output
columns and never see the post-Next() enrichment. ST_CRS(geom)
returned NULL on the consumer side even though the SRID was sitting
in the data.
Fix: pre-read the first record batch in newIPCReaderAdapter, extract
the SRID from the struct's first field, and bake the geoarrow.wkb
CRS metadata into Schema() before returning the adapter. Buffer the
record so the first Next() emits it as if nothing had happened. If
the result set is empty (no batches), fall back to the existing
CRS-less schema.
Includes TestIPCReaderAdapterGeoArrowCRSEager covering the contract:
Schema() must already carry geoarrow.wkb + CRS=EPSG:N before any
Next() call when the first batch's SRID is known.
Verified against a serverless Databricks SQL warehouse via the
streamability harness in duckdb-warehouse-transfer: bigquery -> databricks
now round-trips GEOMETRY('EPSG:4326') byte-perfect with CRS preserved
at the consumer side.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jatorre
force-pushed
the
fix/eager-geoarrow-schema
branch
from
May 22, 2026 11:05
30c4515 to
293e2ca
Compare
Author
|
Note for reviewers: stacks on #411 → #350. It's a small fix that finalizes the |
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
1 participant
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.
Stacked on PR #350 (`feat/arrow-native-geospatial`). Doesn't apply cleanly against upstream main on its own — needs that read-side adapter to land first, or to be rebased onto the merged version of #350.
Summary
When the Arrow-native geospatial read path is enabled, the geoarrow.wkb extension's CRS metadata was being populated on the first record batch by `handleGeoRecord`. But ADBC consumers — DuckDB's `adbc_scanner` in particular — read `Schema()` once at construction time to bind output columns and never see post-Next() schema mutations. Net effect: the destination column comes through as DuckDB `GEOMETRY` (good), but `ST_CRS(geom)` returns NULL even though the SRID was sitting in the data the whole time.
Fix
Pre-read the first record batch in `newIPCReaderAdapter` itself, extract the SRID from the struct's first field, and bake the geoarrow.wkb CRS metadata into `Schema()` before returning the adapter. Buffer the record so the first `Next()` emits it as if nothing had happened. If the result set is empty, fall back to the existing CRS-less schema.
`handleGeoRecord` now only flattens — schema enrichment moves into the constructor.
Test
`TestIPCReaderAdapterGeoArrowCRSEager` covers the contract: `Schema()` must already carry `ARROW:extension:metadata = '{"crs":"EPSG:N"}'` before any `Next()` call when the first batch's SRID is known.
Verified end-to-end against a serverless Databricks SQL warehouse via the streamability harness in `duckdb-warehouse-transfer`: the chained `adbc_insert(databricks, ..., (SELECT * FROM adbc_scan(bigquery, ...)))` round-trips `GEOMETRY('EPSG:4326')` byte-perfect with `ST_CRS(geom) = 'EPSG:4326'` at the consumer.
Test plan
Why now (instead of folding into #350)
Filing as a follow-up keeps each diff focused: #350 wires the option + struct → geoarrow.wkb conversion; this one just fixes the consumer-side ordering bug. Happy to fold them if reviewers prefer.