[GH-3324] Keep other bands' no-data value when clearing one band - #3327
Open
Eliaaazzz wants to merge 5 commits into
Open
[GH-3324] Keep other bands' no-data value when clearing one band#3327Eliaaazzz wants to merge 5 commits into
Eliaaazzz wants to merge 5 commits into
Conversation
…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.
…hrough GeoTIFF round trips The null path consulted band 1's no-data value instead of the target band's, so clearing band N was a no-op whenever band 1 had none. Clearing now also drops the GC_NODATA sentinel from the coverage properties and copies the pixels into a property-free image, since the sentinel rides on the rendered image itself through serialization. RS_AsGeoTiff only writes GDAL_NODATA when the raster actually has a no-data value: GeoTiffWriter otherwise writes a default of 0, which resurrected cleared (and never-set) no-data values on re-read.
…the GeoTIFF nodata opt-out Clearing no longer copies pixels. RenderedImageAdapter cannot mask an inherited property (it declares getProperty final and answers from the source), so PropertyMaskedRenderedImage wraps the image and hides GC_NODATA while delegating everything else. That keeps a lazily decoded raster undecoded and preserves a non-zero image origin, which the previous copyData path lost -- BufferedImage rejects a raster whose minX/minY are not zero. GridCoverage2D.getProperty falls through to the rendered image, so the sentinel is dropped from the coverage property map as well, on a defensive copy so the input raster is left untouched. GeoTools' own CoverageUtilities.getNoDataProperty now reports null for a cleared raster. RS_AsGeoTiff only forces WRITE_NODATA off when no band has a no-data value; forcing it on otherwise would override an explicit -Dgeotiff.writenodata=false.
Clearing a band's no-data value dropped the GC_NODATA sentinel from the coverage properties and the rendered image unconditionally. That sentinel is a single value for the whole coverage, while no-data values are tracked per band on the sample dimensions, so clearing one band also stopped GeoTools operations that read the sentinel from honouring the bands that still declared a no-data value. Jiffle map algebra reads it, so on a two-band raster with no-data 0 on both bands, clearing band 2 changed band 1's result for `out = rast[0] + 1;` from NaN to 1.0. Drop the sentinel only once no band declares a no-data value at all.
GeoTiff stores one GDAL_NODATA value for the whole file, so a raster whose bands disagree about their no-data value cannot be written faithfully: every band reads back with whichever value was written. A two-band raster at (0, null) came back as (0, 0), silently resurrecting a no-data value on a band that had it cleared. Reject the write. Rasters whose bands agree, including rasters with no no-data value at all, are unaffected.
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
2 participants
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.
Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-XXX] my subject. Closes Clearing NoData on one raster band changes another band's MapAlgebra results #3324What changes were proposed in this PR?
Two independent commits, so the second can be dropped without losing the first.
1. Keep other bands' no-data value when clearing one band (
309b16a)No-data values are tracked per band on the sample dimensions.
GC_NODATAis a single sentinel for the whole coverage, carried both by the coverage properties and by the rendered image. The clear path dropped that sentinel unconditionally, so clearing one band also stopped GeoTools operations that read the sentinel from honouring the bands that still declared a no-data value. Jiffle reads it, which is how clearing band 2 changed band 1'sout = rast[0] + 1;fromNaNto1.0.The fix drops the sentinel only once no band declares a no-data value at all. Reproduced before the change and verified after, on the exact steps in the issue:
0.0, band 20.0,rast[0] + 1givesNaN.0.0, band 2null,rast[0] + 1gives1.0. This is the bug.0.0, band 2null,rast[0] + 1givesNaN.null, band 2null,rast[0] + 1gives1.0.2. Reject mixed per-band no-data values when writing GeoTiff (
9e5525c)This is the second half of the issue: a
(0, null)raster came back from a GeoTIFF round trip as(0, 0). GDAL_NODATA is one value for the whole file, so that state cannot be written faithfully.RS_AsGeoTiffnow throwsIllegalArgumentExceptionon it, rather than resurrecting a no-data value on a band that had it cleared.This commit needs a maintainer decision, because its blast radius is wider than the clear path.
RasterUtils.copyRasterAndAppendBandsets the appended band's no-data value and leaves the existing bands' sample dimensions alone, soRS_AddBandandRS_Unionroutinely produce rasters whose bands disagree. Writing one of those to GeoTIFF silently rewrites the other bands' no-data today; with this commit it fails loudly. That follows the issue's preference for rejecting unsupported mixed states, but if it reads as too strong forRS_AsGeoTiff, dropping9e5525cleaves commit 1 intact and I will open a separate issue for the writer.How was this patch tested?
New tests in
common:RasterBandEditorsTest.testSetBandNoDataValueWithNullKeepsNoDataOnOtherBandscovers the issue's reproducer. It checks both bands and map algebra after clearing either one, before serialization, and that the input raster is not mutated.RasterBandEditorsTest.testSetBandNoDataValueWithNullOnEveryBandClearsNoDataPropertycovers the case where every band is cleared: the sentinel is dropped from the coverage and the image, map algebra reads the zeros as ordinary data, and the state survives a GeoTIFF round trip.RasterOutputTest.testAsGeoTiffRejectsBandsWithDifferentNoDataValuescovers "only one band set" and "both set but different". Rasters whose bands agree, and rasters with no no-data value at all, still round-trip.mvn -pl common testwas run before and after the change on Windows with JDK 17. The same 13 tests fail both times:RasterBandEditorsTest.testClip, 5RasterEditorsTest.testResample*, and 7RasterOutputTest.testAsMatrix*. They fail identically on unmodified2b06236, so they are pre-existing on this platform and unrelated to this change. Everything else passes, including the three new tests.I could not run
spark/commonlocally. It fails to compile on the generated OSM PBF protobuf sources (package proto4 does not exist) before reaching any raster code, so those suites are left to CI.One note for whoever reviews the tests.
MapAlgebracaches a compiled Jiffle runtime per script text in aThreadLocal, so I checked whether a cached runtime could mask this regression by carrying a previous raster's no-data handling into a later call with the same script. It does not:setSourceImagere-reads the property. The tests therefore use a plain shared script.Did this PR include necessary documentation updates?
RS_SetBandNoDataValuenow states that no-data is tracked per band, andRS_AsGeoTiffdocuments the single-value-per-file constraint and the new rejection.🤖 Generated with Claude Code
https://claude.ai/code/session_01P6hpqAnXfeaWRLkL1VQDMd