Skip to content

feat(rust/sedona-raster-functions): add RS_ReplaceBandNoDataValue - #1343

Merged
james-willis merged 10 commits into
apache:mainfrom
james-willis:jw/rs-setbandnodatavalue-replace
Oct 2, 2026
Merged

james-willis merged 10 commits into
apache:mainfrom
james-willis:jw/rs-setbandnodatavalue-replace

Conversation

@james-willis

@james-willis james-willis commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Adds RS_ReplaceBandNoDataValue(raster, band, nodata). Every pixel of the band that reads as nodata is rewritten to nodata, which then becomes the band's nodata value, so the same pixels read as nodata before and after the call. RS_SetBandNoDataValue only changes the declared value. If no pixel holds the old nodata value, the band's bytes are kept as they are without a copy, and only the declared value changes.

This PR started as Sedona Spark's 4-argument RS_SetBandNoDataValue(raster, band, nodata, replace) form. Per review, it's now a separate function instead. That also resolves the needs_pixels problem: the flag is per UDF, so the replace kernel made every RS_SetBandNoDataValue call load pixels. RS_SetBandNoDataValue is back to metadata only, unchanged from main. Sedona Spark gets the same function and loses the 4-argument form in apache/sedona#3424 (issue apache/sedona#3423), so both engines keep one API.

Design

  • Planner flags: the function is tagged needs_pixels and returns_bytes, so its output can feed RS_Value, RS_SummaryStats or another RS_ReplaceBandNoDataValue without being wrapped in a second RS_EnsureLoaded. The earlier version failed that way.
  • Nodata matching: it uses the shared NodataMatcher, the rules sampling uses. Integer bands compare exactly. Float bands compare numerically, so -0.0 matches a 0.0 nodata and a NaN pixel with any payload matches a NaN nodata. The earlier byte-equality version left those pixels unmasked. Sedona Spark's RasterUtils.isNoData uses the same rule.
  • Reading pixels: the band is gathered through the shared scan_pixels, so strided, reversed and broadcast views work, not just contiguous bands. The packed size is checked before allocating. 2-D bands only, as for the other pixel-reading functions.
  • Explicit band only. Sedona Spark's 2-argument setter forms default to band 1 where SedonaDB's reject a multiband raster (the test_rs_setbandnodata_two_arg_multi_band xfail), and a new function need not inherit that.
  • Errors and nulls: a band with no nodata value to replace is an error, as in Sedona Spark. So is a nodata value the band's type can't hold exactly (nodata_f64_to_bytes). A null raster, band or nodata gives a null raster.

The first commit, the SEDONADB_SEDONA_SPARK_JARS hook for running the parity suite against a locally built Sedona jar, is unchanged. It's deliberately not wired into CI.

Parity

The two replace cases (single band and multiband, both anchored to the correct raster) move to test_rs_replacebandnodatavalue.py. They xfail against the pinned Sedona 1.9.1, which has no such function, and flip green once a release carries apache/sedona#3424. The rest of test_rs_setbandnodatavalue.py is unchanged apart from the updated null_value reason.

Verification

  • cargo test -p sedona-raster-functions --lib: all passing, 8 of them new for this function. Clippy is clean.
  • python/sedonadb/tests/functions/test_rs_replacebandnodatavalue.py (new, including the nested-call regression) and test_raster_functions.py: 80 passed.
  • The parity files touched here with -o xfail_strict=true: 12 passed, 8 xfailed.

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a separate function (RS_ReplaceBandNoDataValue()?), which would solve the "needs bytes" issue in addition to being a cleaner API.

Comment thread rust/sedona-raster-functions/src/rs_set_band_nodata.rs Outdated
@james-willis

Copy link
Copy Markdown
Contributor Author

@jiayuasu what do you think. I prefer Dewey's proposal to just porting the spark behavior

…ness

Sedona publishes no usable snapshot to Maven — the Apache Nexus snapshots
stop at 1.8.1-SNAPSHOT (last updated 2025-09-09) and carry no Spark 4
artifact — so testing against an unreleased fix means a jar built by its CI.
SEDONADB_SEDONA_SPARK_JARS points spark.jars at one; narrow
SEDONADB_SEDONA_SPARK_PACKAGES alongside it to the dependencies the shaded
jar does not bundle.
…rload

Adds the 4-argument form Sedona Spark has,
RS_SetBandNoDataValue(raster, band, nodata, replace). With replace true,
every pixel in the addressed band holding that band's current nodata is
rewritten to the new sentinel before it is declared, so the pixels that read
as nodata before the call still read as nodata after it. Other bands are
untouched, and replacing against a band with no existing nodata is an error —
there is nothing to match, and Sedona Spark raises there too. A null nodata
still clears the band, which leaves no sentinel to rewrite pixels to, so
replace is moot and no pixel is touched.

The match is on raw little-endian bytes rather than decoded values, which is
exact for every band data type without dispatching over them: the new sentinel
arrives already packed into the band's type by nodata_f64_to_bytes, so both
sides are the same width by construction. The one inherited behavior is that a
NaN sentinel matches only pixels carrying the identical NaN bit pattern, where
Sedona Spark compares numerically and NaN never matches itself.

Because it rewrites pixels, this form cannot share the source buffer the way
the metadata-only forms do, so the UDF is now tagged needs_pixels and the
planner materializes its raster argument through RS_EnsureLoaded. That is what
makes it work on an OutDb raster, which is what the parity fixtures read; the
cost is that the metadata-only forms are materialized too, where previously
they passed an OutDb raster through untouched.

The parity suite's single-band replace case now agrees on the released Sedona
jar and loses its xfail. The multiband case still trips apache/sedona#3330,
where Sedona Spark zeroes every band except the target; that is fixed on
Sedona master by apache/sedona#3347 and verified there against a jar built
from it, but no release carries it yet, so the case keeps an xfail naming the
release it waits on.
@jiayuasu

Copy link
Copy Markdown
Member

Sounds good to me.

Comment thread rust/sedona-raster-functions/src/rs_set_band_nodata.rs Outdated
Comment thread rust/sedona-raster-functions/src/rs_set_band_nodata.rs Outdated
…ReplaceBandNoDataValue

Replace the 4-argument RS_SetBandNoDataValue(raster, band, nodata, replace)
form with its own function, RS_ReplaceBandNoDataValue(raster, band, nodata),
as suggested in review. RS_SetBandNoDataValue goes back to metadata only and
no longer asks the planner to load pixels; only the new function is tagged
needs_pixels, together with returns_bytes so its output can feed another
pixel-reading function without being wrapped twice.

The new function gathers the band through the shared pixel scan, so strided
and broadcast views work, and matches the old nodata with NodataMatcher, the
rules sampling uses: -0.0 matches a 0.0 nodata and any NaN matches a NaN
nodata. It checks the packed size before allocating. It takes an explicit
band only.

Sedona Spark gets the same function in apache/sedona#3423; the parity cases
xfail against the pinned 1.9.1 jar until a release carries it.
@james-willis james-willis changed the title feat(rust/sedona-raster-functions): RS_SetBandNoDataValue replace overload feat(rust/sedona-raster-functions): add RS_ReplaceBandNoDataValue Sep 28, 2026
james-willis added a commit to james-willis/sedona that referenced this pull request Sep 30, 2026
…e into RS_ReplaceBandNoDataValue

Remove the 4-argument RS_SetBandNoDataValue(raster, band, noDataValue,
replace) form and add RS_ReplaceBandNoDataValue(raster, band, noDataValue)
for the replace behaviour: pixels holding the band's current no-data value
are rewritten to the new value before it is declared, so they keep reading
as no-data. RS_SetBandNoDataValue is metadata-only again. The Java method
RasterBandEditors.setBandNoDataValue(raster, band, value, replace) becomes
replaceBandNoDataValue(raster, band, value).

SedonaDB is adding the same function (apache/sedona-db#1343), so both
engines keep one API.
@james-willis
james-willis marked this pull request as ready for review September 30, 2026 18:40

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Possible optimization to handle now or later.

Comment on lines +225 to +228
reason="needs a Sedona release carrying apache/sedona#3312 — the released "
"1.9.1 jar propagates the NULL and returns a NULL raster where SedonaDB "
"clears the band's nodata (the trinary override semantics from #1198); "
"Sedona master now clears it too, verified against a jar built from it"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, I missed these in review and I think that messing with null propagation isn't worth it. Using another function name (RS_UnSetCrs(...)) or a sentinel (RS_SetCrs(..., zap())) is cleaner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your comment is about CRSes but I assume you mean the same thing when talking about NoDataValue?

Comment thread rust/sedona-raster-functions/src/rs_replace_band_nodata.rs
Comment on lines +217 to +222
fn i64_t() -> SedonaType {
SedonaType::Arrow(DataType::Int64)
}
fn f64_t() -> SedonaType {
SedonaType::Arrow(DataType::Float64)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you inline these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to get the agent to behave better here. probably will make a UDF skill based on my DEWEY.md file 😆

@jiayuasu jiayuasu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

…bytes when no pixel holds nodata

Scan for a nodata pixel before copying the band: when there is none, only
the declared nodata value changes and the band's bytes are carried over
without a copy. Inline the test module's type helpers.
…s test calls

Each test builds its own tester and calls the function directly, instead of
going through a shared wrapper.
@james-willis
james-willis merged commit 0fdc4b8 into apache:main Oct 2, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants