Spark 4.1: Test geometry and geography DML and fall back from vectorized reads - #17149
Conversation
…zed reads Add end-to-end DELETE/UPDATE/MERGE tests for geometry and geography columns under merge-on-read at format version 3, covering deletion vectors, nested geo in a struct, and null values. Geometry and geography are primitive types, so they slipped through the vectorized Parquet read gate in SparkBatch even though there is no Arrow geo vector yet, which threw when scanning geo columns with vectorization enabled. Exclude the two geo types so they fall back to the non-vectorized reader.
Select the delete snapshot by operation instead of the latest committed_at, whose millisecond granularity could tie with the insert snapshot, and give the test its own table so there is exactly one delete snapshot to inspect.
Copy-on-write is the default row-level mode, and its rewrite path reads geo values back out of the surviving data files, so add DELETE and UPDATE tests that exercise it (the merge-on-read tests only cover deletion vectors).
Fold the duplicated merge-on-read and copy-on-write DELETE and UPDATE variants into single tests parametrized over the write mode, and run MERGE and the nested delete over both modes too. The deletion-vector assertion applies only to merge-on-read.
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
Reviewed the geo DML tests and the vectorization fallback fix. Overall this looks good: the production change is minimal and correct (geometry/geography are PrimitiveTypes so they wrongly passed the batch-read gate and hit ArrowSchemaUtil's Unsupported primitive type throw; excluding them at the gate is the right layer, mirroring how VARIANT stays non-vectorized). The tests are thorough — DELETE/UPDATE/MERGE parametrized over copy-on-write and merge-on-read, COALESCE(1) to force a DV in MOR, null round-trips, and testGeospatialWkbReadBack un-skipped for vectorization on. CI is green. A few non-blocking nits and one question below.
(Review drafted with AI assistance and reviewed before posting.)
| Type type = field.type(); | ||
| // Geometry and geography are primitive types but have no Arrow vector yet, so they must be | ||
| // read through the non-vectorized reader. | ||
| if (type.typeId() == Type.TypeID.GEOMETRY || type.typeId() == Type.TypeID.GEOGRAPHY) { |
There was a problem hiding this comment.
Correct fix and the right layer. Nit: the method-level comment above useParquetBatchReads() still reads // - only primitives or metadata columns are projected, which is now slightly stale since geometry/geography are primitives that are intentionally excluded here. A one-line tweak there would keep the two in sync.
There was a problem hiding this comment.
Done in 3698f8c — updated the comment above useParquetBatchReads() to note geometry and geography are primitives that are intentionally excluded.
|
|
||
| @ParameterizedTest | ||
| @ValueSource(strings = {"copy-on-write", "merge-on-read"}) | ||
| public void testDeleteGeospatial(String mode) { |
There was a problem hiding this comment.
Nit: these DML tests rely on the default read.parquet.vectorization.enabled=true to exercise the fallback path. That's fine today, but parametrizing vectorization on/off (like testGeospatialWkbReadBack does) would make the intent explicit and guard against a future default change. Optional.
There was a problem hiding this comment.
Good call — done in 3698f8c. The DELETE/UPDATE/MERGE/nested tests now parametrize over (mode, vectorized) via @CsvSource, setting read.parquet.vectorization.enabled per table, so the fallback is exercised explicitly with vectorization both on and off rather than relying on the default.
| if (mode.equals("merge-on-read")) { | ||
| assertThat( | ||
| scalarSql( | ||
| "SELECT summary['added-dvs'] FROM %s.snapshots WHERE operation = 'delete'", |
There was a problem hiding this comment.
Nit: the DV assertion queries the snapshots metadata view via SQL. That works, and the operation = 'delete' filter nicely avoids the committed_at tie. Elsewhere in Spark 4.1 tests (e.g. TestDeleteFrom.truncateWithDVs) the convention is to load the table via the catalog and assert on snapshot().summary() with SnapshotSummary.ADDED_DVS_PROP, which is a bit more direct and avoids depending on the snapshots view. Either is acceptable.
There was a problem hiding this comment.
Thanks, that's a fair point. I looked at how other tests in this package handle it: TestSelect (same spark/.../sql/ package) also reads the .snapshots metadata view via SQL, whereas the currentSnapshot().summary() + SnapshotSummary.ADDED_DVS_PROP convention you mention is used by the CatalogTestBase tests (e.g. TestDeleteFrom), which have validationCatalog/tableIdent available. This test extends TestBase and creates its table via SQL in a hadoop catalog (Hive doesn't support geo yet), so there's no validationCatalog to load from, and the SQL form keeps it consistent with the sibling tests here. I've kept the SQL assertion with the operation = 'delete' filter for determinism — but happy to switch to the catalog form if you'd prefer it.
|
|
||
| @ParameterizedTest | ||
| @ValueSource(strings = {"copy-on-write", "merge-on-read"}) | ||
| public void testDeleteNestedGeometry(String mode) { |
There was a problem hiding this comment.
Question: this nested case covers GEOMETRY inside a struct but not nested GEOGRAPHY. Intentional scope for Phase 1, or worth a symmetric geography case?
There was a problem hiding this comment.
Good catch — added in 3698f8c. The nested test (now testDeleteNestedGeospatial) uses STRUCT<c1, geom, geog> and asserts that after the delete the surviving row's non-null nested geometry and null nested geography both round-trip.
Parametrize the DELETE/UPDATE/MERGE/nested tests over vectorization on and off (in addition to the row-level mode) so the fallback path is exercised explicitly rather than relying on the default. Extend the nested-delete test to a struct that also holds a geography field, asserting a non-null nested geometry and a null nested geography both round-trip. Sync the useParquetBatchReads comment to note geometry and geography are excluded.
|
Thanks for the review @szehon-ho! Addressed in 3698f8c:
On the DV assertion: kept the SQL form since this test extends |
|
Merged, thanks @huan233usc ! |
3 participants
Adds end-to-end Spark SQL DML tests for
geometryandgeographycolumns, andfixes a vectorized-read bug those tests surfaced. This completes the Phase-1
geo end-to-end item (row-level DML with deletion vectors / nested geo / nulls)
and builds on #17073 (Spark geo value path) and #16982 (Parquet geo WKB values).
Tests
TestSparkGeospatialpreviously covered onlyINSERT→SELECTread-back. Thisadds row-level DML on a geo table under merge-on-read + format version 3
(deletion vectors):
testDeleteGeospatialMergeOnRead—DELETEleaves a survivor in a single datafile, and the snapshot summary confirms a deletion vector was written
(
added-dvs = 1); the surviving null-geo row round-trips.testUpdateGeospatialMergeOnRead—UPDATEswaps a row's geometry; othersunchanged.
testMergeGeospatialMergeOnRead—MERGEupdates a matched row and inserts anot-matched row, both carrying geo values.
testDeleteNestedGeometryMergeOnRead— geometry nested in aSTRUCT; delete bya nested non-geo field, surviving nested geometry (and a null nested geometry)
still round-trip.
Geo values are asserted via
hex(st_asbinary(...))round-trips and DML filters onnon-geo columns, since Spark 4.1 has no spatial predicates.
Vectorization fallback (production fix)
geometry/geographyare primitive types, so they passedSparkBatch.supportsParquetBatchReads(NestedField)and were routed to thevectorized Parquet reader — but there is no Arrow geo vector yet, so scanning a geo
column with vectorization enabled threw
UnsupportedOperationException: Unsupported primitive type: geometry. This excludesthe two geo types from vectorized reads so they fall back to the row-based reader.
With this fix,
testGeospatialWkbReadBacknow runs (no longer skipped) withvectorization both off and on, proving the fallback. A dedicated Arrow geo
vector remains future work.