[GH-3311] Make RS_SetBandNoDataValue honor NULL to remove a band's no-data value in Spark SQL by james-willis · Pull Request #3312 · apache/sedona

james-willis · 2026-09-02T16:41:24Z

Did you read the Contributor Guide?

Yes, I have read the Contributor Rules and Contributor Development Guide

Is this PR related to a ticket?

Yes, and the PR name follows the format [GH-XXX] my subject. Closes RS_SetBandNoDataValue: documented "NULL removes the no-data value" behavior is unreachable from Spark SQL #3311

What changes were proposed in this PR?

Scoped down to just making a NULL noDataValue behave as documented. The GC_NODATA property work that grew out of the review is split into a follow-up, since it turned out to be a separate problem with its own failure modes (see the end of this description).

Make the documented NULL behavior reachable from SQL. The docs and the common Java implementation both say a null noDataValue removes the band's no-data value, but from Spark SQL that path was unreachable: the catalyst expression wrapped RasterBandEditors.setBandNoDataValue with inferrableFunction2/3/4, which null-propagate as soon as any argument is null, so RS_SetBandNoDataValue(raster, 1, NULL) returned a NULL raster instead. This matches PostGIS ST_SetBandNoDataValue NULL-clears semantics and the behavior SedonaDB adopted in apache/sedona-db #1158.

Added InferrableFunction.allowRightNull3, an arity-3 counterpart of the existing allowRightNull pattern (allows the third argument to be null; still null-propagates when the raster or band index is null), and wired the 2- and 3-argument overloads through it. It is a separate name rather than an overload because overloading allowRightNull breaks eta-expansion of the overloaded functions passed to it at existing call sites. The 4-argument replace overload still null-propagates, since replacing pixels with a null no-data value is meaningless; the docs now say so.

Clear the target band, not band 1. The null path consulted band 1's no-data value to decide whether there was anything to remove, so clearing band N was a no-op whenever band 1 had none. Latent until now because the path was unreachable from SQL and the Java tests only ever cleared band 1.

Stop RS_AsGeoTiff inventing a no-data value. GeoTiffWriter writes a default GDAL_NODATA of 0 when a coverage has no no-data value, so a raster that never had one came back from a write/read round trip reporting 0. This is independent of NULL handling, but it is what made a cleared no-data value reappear on the round trip, so it is fixed here: WRITE_NODATA is set to false in that case only, leaving an explicit -Dgeotiff.writenodata=false untouched.

How was this patch tested?

The existing RS_SetBandNoDataValue test could not distinguish clearing from null-propagation — RS_BandNoDataValue returns null for a null raster too, so it passed under both semantics. It now sets a no-data value, clears it with NULL through both the 2- and 3-argument forms, and asserts the resulting raster is non-null while RS_BandNoDataValue on it is null; a NULL raster input still yields NULL.

New coverage for the other two fixes: clearing band 2 of a raster whose band 1 has no no-data value (both at the Java and SQL levels), a SQL round trip asserting a cleared value does not come back through RS_AsGeoTiff, and two RasterOutputTest cases pinning that a raster without a no-data value round-trips without gaining one while a raster with one keeps it.

Each new assertion fails without its fix. Full runs: common 1339 passed, rasteralgebraTest 168 passed, rasterIOTest 21 passed.

Did this PR include necessary documentation updates?

Yes, I have updated the documentation.

Follow-up, not in this PR

GC_NODATA is a single coverage-wide property while no-data is declared per band, and the two are not kept in sync in either direction. This PR deliberately does not touch that:

Setting a no-data value only writes the band's sample dimension. On a raster with an existing GC_NODATA the declared value is then ignored by anything reading the property — RS_MapAlgebra keeps using the old sentinel. On a raster without one, nothing creates it, so a freshly set no-data value has no effect on RS_MapAlgebra at all.
Clearing has the mirror problem, and removing the property is not a safe fix on its own: because it is coverage-wide, dropping it while another band still declares that value changes that band's results (Clearing NoData on one raster band changes another band's MapAlgebra results #3324).
A GeoTIFF also cannot represent a heterogeneous per-band state — the container has one GDAL_NODATA slot, and GDAL itself spills a divergent per-band value into a .aux.xml sidecar rather than the TIFF, which RS_AsGeoTiff has no equivalent of since it returns bytes.

Keeping the property in step with the sample dimensions in both directions, and deciding what RS_AsGeoTiff should do with a heterogeneous state, belongs in a separate PR.

…e in Spark SQL The docs and the common Java implementation say a null noDataValue removes the band's no-data value, but the catalyst expression null-propagated on any null argument, so RS_SetBandNoDataValue(raster, 1, NULL) returned a null raster and the documented clear path was unreachable from SQL. Route the 2- and 3-arg overloads through the allowRightNull pattern (adding an arity-3 variant) so a null noDataValue reaches the Java implementation. The 4-arg replace overload still null-propagates since replacing pixels with a null no-data value is meaningless.

The null path consulted band 1's no-data value to decide whether there was anything to remove, so clearing band N was a no-op whenever band 1 had none. It went unnoticed because the path was unreachable from SQL and the Java tests only ever cleared band 1.

GeoTiffWriter writes a default GDAL_NODATA of 0 when a coverage has no no-data value, so writing and re-reading a raster invented one: a raster that never had a no-data value came back reporting 0. That also made a cleared no-data value reappear on the round trip. Set WRITE_NODATA to false in that case only, leaving an explicit -Dgeotiff.writenodata=false untouched.

james-willis · 2026-09-08T23:43:06Z

I simplified this PR. The issues with bandwise no data values, GC_NODATA persisting after SetNoDataValueCalls, and writing geotiff No Data Values are orthogonal to this PR and thus out of scope.

They are best handled separately.

james-willis force-pushed the fix/rs-setbandnodatavalue-null-clear branch from 3768356 to ab5f6a3 Compare September 2, 2026 17:26

james-willis marked this pull request as ready for review September 2, 2026 17:26

james-willis requested a review from jiayuasu as a code owner September 2, 2026 17:26

james-willis assigned jiayuasu Sep 2, 2026