jiayuasu commented on code in PR #3114:
URL: https://github.com/apache/sedona/pull/3114#discussion_r3660788940
##########
common/src/main/java/org/apache/sedona/common/raster/RasterConstructors.java:
##########
@@ -130,8 +131,35 @@ public static GridCoverage2D asRaster(
boolean useGeometryExtent)
throws FactoryException {
+ // Reject a burn value that cannot be represented in the target pixel
type, for the same reason
+ // the noDataValue is rejected below: silently coercing an out-of-range or
fractional value (for
+ // example 265 -> 9 in an unsigned 8-bit band) would store a different
number than the caller
+ // asked to burn. Unlike noDataValue this is a primitive double, so it is
always validated.
+ RasterUtils.assertRepresentable(value, pixelType, "value");
Review Comment:
I think reusing the nodata check here is too strict. `value` is the burn
value, not the nodata sentinel, so normal conversion to the band type seems
expected. For example, PostGIS accepts this:
```sql
WITH r AS (
SELECT ST_AsRaster(
ST_GeomFromText('POLYGON((0 0,2 0,2 2,0 2,0 0))'),
2, 2, '32BF'::text, 0.1::float8, 0.5::float8
) AS rast
)
SELECT ST_Value(rast, 1, 1, 1, false) FROM r;
-- 0.10000000149011612
```
GDAL does the same Float32 rounding. Could we keep the exact check for
`noDataValue`, but use a separate burn-value check? Rejecting out-of-range
integer burns may still be useful; rejecting `0.1` for `F` seems surprising.
##########
common/src/main/java/org/apache/sedona/common/raster/RasterConstructors.java:
##########
@@ -130,8 +131,35 @@ public static GridCoverage2D asRaster(
boolean useGeometryExtent)
throws FactoryException {
+ // Reject a burn value that cannot be represented in the target pixel
type, for the same reason
+ // the noDataValue is rejected below: silently coercing an out-of-range or
fractional value (for
+ // example 265 -> 9 in an unsigned 8-bit band) would store a different
number than the caller
+ // asked to burn. Unlike noDataValue this is a primitive double, so it is
always validated.
+ RasterUtils.assertRepresentable(value, pixelType, "value");
+
+ // Reject a noDataValue that cannot be represented in the target pixel
type rather than
+ // silently coercing it: writing an out-of-range or fractional value into
an integer sample
+ // stores a different number than the value recorded as the band's nodata
metadata, so the
+ // background would read back as data. Validating once here keeps the
background fill and the
+ // nodata metadata below using the same value.
+ if (noDataValue != null) {
+ noDataValue = RasterUtils.assertNoDataValueRepresentable(noDataValue,
pixelType);
+ }
+
+ // If the burn value equals the noDataValue, every covered pixel reads
back as nodata, so the
+ // geometry would vanish from the output. Reject rather than produce an
all-nodata raster; this
+ // also catches the default burn value of 1 colliding with an inherited
nodata of 1.0.
+ if (noDataValue != null && Double.compare(value, noDataValue) == 0) {
Review Comment:
I am not sure we need to reject this combination. PostGIS allows the burn
value and nodata value to be equal:
```sql
WITH r AS (
SELECT ST_AsRaster(
ST_GeomFromText('POLYGON((0 0,2 0,2 2,0 2,0 0))'),
2, 2, '32BF'::text, 1::float8, 1::float8
) AS rast
)
SELECT
ST_Value(rast, 1, 1, 1, false),
ST_Value(rast, 1, 1, 1, true)
FROM r;
-- 1 | NULL
```
That is an all-nodata raster, but it can be intentional. This check also
makes the shorter overload fail whenever the reference nodata is `1`, since the
default burn value is also `1`. I would lean toward allowing the caller to
choose this.
--
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]
