jiayuasu commented on code in PR #3312:
URL: https://github.com/apache/sedona/pull/3312#discussion_r3922403638
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/raster/RasterBandEditors.scala:
##########
@@ -22,13 +22,15 @@ import org.apache.sedona.common.raster.RasterBandEditors
import org.apache.spark.sql.catalyst.expressions.Expression
import
org.apache.spark.sql.sedona_sql.expressions.InferrableFunctionConverter._
import org.apache.spark.sql.sedona_sql.expressions.InferrableRasterTypes._
-import org.apache.spark.sql.sedona_sql.expressions.InferredExpression
+import org.apache.spark.sql.sedona_sql.expressions.{InferrableFunction,
InferredExpression}
private[apache] case class RS_SetBandNoDataValue(inputExpressions:
Seq[Expression])
extends InferredExpression(
inferrableFunction4(RasterBandEditors.setBandNoDataValue),
- inferrableFunction3(RasterBandEditors.setBandNoDataValue),
- inferrableFunction2(RasterBandEditors.setBandNoDataValue)) {
+ // A null noDataValue removes the band's no-data value, so let it
through instead of
+ // null-propagating the whole expression.
+ InferrableFunction.allowRightNull3(RasterBandEditors.setBandNoDataValue),
Review Comment:
Could this use the `rasterNoData` value read for `bandIndex`? The null path
in the common implementation currently calls `getBandNoDataValue(raster)`,
which checks band 1. I think this leaves band 2 unchanged when band 1 has no
NoData:
```scala
val df = sparkSession.sql(
"SELECT RS_MakeEmptyRaster(2, 20, 20, 0, 0, 8, 8, 0.1, 0.1, 0) AS raster")
val result = df
.selectExpr("RS_SetBandNoDataValue(raster, 2, 444) AS raster")
.selectExpr("RS_BandNoDataValue(RS_SetBandNoDataValue(raster, 2, NULL),
2)")
.first().getDouble(0)
// 444.0
```
I expected the last value to be null. An asymmetric multi-band test may be
useful here.
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/raster/RasterBandEditors.scala:
##########
@@ -22,13 +22,15 @@ import org.apache.sedona.common.raster.RasterBandEditors
import org.apache.spark.sql.catalyst.expressions.Expression
import
org.apache.spark.sql.sedona_sql.expressions.InferrableFunctionConverter._
import org.apache.spark.sql.sedona_sql.expressions.InferrableRasterTypes._
-import org.apache.spark.sql.sedona_sql.expressions.InferredExpression
+import org.apache.spark.sql.sedona_sql.expressions.{InferrableFunction,
InferredExpression}
private[apache] case class RS_SetBandNoDataValue(inputExpressions:
Seq[Expression])
extends InferredExpression(
inferrableFunction4(RasterBandEditors.setBandNoDataValue),
- inferrableFunction3(RasterBandEditors.setBandNoDataValue),
- inferrableFunction2(RasterBandEditors.setBandNoDataValue)) {
+ // A null noDataValue removes the band's no-data value, so let it
through instead of
+ // null-propagating the whole expression.
+ InferrableFunction.allowRightNull3(RasterBandEditors.setBandNoDataValue),
+ InferrableFunction.allowRightNull(RasterBandEditors.setBandNoDataValue))
{
Review Comment:
Could we also clear the image-level `GC_NODATA` metadata? On
`raster_with_no_data/test5.tiff`, this round trip seems to bring `0` back:
```scala
val result = sparkSession.read.format("binaryFile")
.load(resourceFolder + "raster/raster_with_no_data/test5.tiff")
.selectExpr("RS_SetBandNoDataValue(RS_FromGeoTiff(content), NULL) AS
raster")
.selectExpr("RS_AsGeoTiff(raster) AS bytes")
.selectExpr("RS_BandNoDataValue(RS_FromGeoTiff(bytes)) AS nodata")
.first().getDouble(0)
// 0.0
```
Before serialization the accessor reports null, but `clone(..., keepMetadata
= true)` appears to preserve `GC_NODATA=0`. I think removing or recomputing
that property along with the sample dimension would avoid downstream readers
treating zero as NoData.
--
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]
