huan233usc commented on code in PR #17149:
URL: https://github.com/apache/iceberg/pull/17149#discussion_r3566679543
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkGeospatial.java:
##########
@@ -92,6 +94,159 @@ public void testGeospatialWkbReadBack(boolean vectorized) {
TABLE));
}
+ @ParameterizedTest
+ @ValueSource(strings = {"copy-on-write", "merge-on-read"})
+ public void testDeleteGeospatial(String mode) {
+ // Exercise both row-level modes: copy-on-write (the default) rewrites the
surviving rows into a
+ // new data file, while merge-on-read writes a deletion vector. Both must
read the geo values
+ // back out correctly. Write both rows into one data file (COALESCE(1)) so
the merge-on-read
+ // delete leaves a survivor in that file, forcing a deletion vector rather
than a whole-file
+ // removal. Filter on id since Spark has no spatial predicate.
+ String deleteTable = CATALOG + ".default.geo_delete";
+ sql("DROP TABLE IF EXISTS %s", deleteTable);
+ sql(
+ "CREATE TABLE %s (id BIGINT, geom GEOMETRY(4326), geog
GEOGRAPHY(4326)) USING iceberg "
+ + "TBLPROPERTIES ('format-version'='3', 'write.delete.mode'='%s')",
+ deleteTable, mode);
+ sql(
+ "INSERT INTO %s SELECT /*+ COALESCE(1) */ id, "
+ + "CASE WHEN geom_wkb IS NULL THEN NULL "
+ + "ELSE st_setsrid(st_geomfromwkb(geom_wkb), 4326) END, "
+ + "CASE WHEN geog_wkb IS NULL THEN NULL "
+ + "ELSE st_setsrid(st_geogfromwkb(geog_wkb), 4326) END "
+ + "FROM VALUES (1, X'%s', X'%s'), (2, CAST(NULL AS BINARY),
CAST(NULL AS BINARY)) "
+ + "AS v(id, geom_wkb, geog_wkb)",
+ deleteTable, GEOM_WKB, GEOG_WKB);
+
+ sql("DELETE FROM %s WHERE id = 1", deleteTable);
+
+ assertEquals(
+ "Only the null-geo row should remain after the delete",
+ ImmutableList.of(row(2L, null, null)),
+ sql(
+ "SELECT id, hex(st_asbinary(geom)), hex(st_asbinary(geog)) FROM %s
ORDER BY id",
+ deleteTable));
+
+ // A merge-on-read delete of a row from a multi-row file writes a deletion
vector (format v3),
+ // recorded on the 'delete' snapshot; copy-on-write instead rewrites the
file under an
+ // 'overwrite' snapshot and adds no deletion vector. Select the
merge-on-read delete snapshot by
+ // operation so the assertion does not depend on committed_at ordering,
whose millisecond
+ // granularity could tie with the insert snapshot.
+ if (mode.equals("merge-on-read")) {
+ assertThat(
+ scalarSql(
+ "SELECT summary['added-dvs'] FROM %s.snapshots WHERE
operation = 'delete'",
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
