test(integration): compare() accepts an optional expected anchor - #1218
Merged
Conversation
…f hand-rolling it
Engines agreeing with each other is not proof either is right — every engine returning the same wrong thing passes the parity claim. compare() now takes an optional keyword-only expected= asserted on the subject first (references are anchored transitively): anything assert_result accepts for table results, a DecodedRaster for raster results. Anchor the tests whose expected value is trivially known: the getter test (written nodata and NULL) and all four passing setter tests (the exact seeded pixels, grid, and per-band nodata) — the setter tests would previously have passed if both engines no-opped identically.
Includes the commits of apache#1211 (bbox placement in the raster fixture writers) and converts the anchored tests' explicit grid placement to bbox=. The anchors keep their GDAL transform tuples — that is the decoded representation assert_decoded_equal compares, not a placement choice.
Same exactly-one bbox/gdal_transform surface as the write_* helpers, so anchors can state a north-up grid readably. The instance always carries a gdal_transform afterwards (decoders produce transforms and assert_decoded_equal compares them); bbox is construction sugar, derived from the pixel shape with rasterio's from_bounds.
DecodedRaster now takes the same exactly-one bbox/gdal_transform surface as the writers (apache#1211), so the anchors state the same BBOX the fixtures are placed with and the duplicate transform constant goes away.
This was referenced Sep 2, 2026
paleolimbot
approved these changes
Sep 2, 2026
Comment on lines
+47
to
+64
| # The grid the anchored tests write and reconstruct. Anchors must state the | ||
| # exact raster they expect, so the placement is passed to | ||
| # create_random_raster_view explicitly rather than relying on its defaults | ||
| # staying in sync — and the anchors state the same BBOX. | ||
| BANDS, HEIGHT, WIDTH = 2, 6, 7 | ||
| BBOX = (100.0, 482.0, 114.0, 500.0) | ||
|
|
||
|
|
||
| def _anchor(dtype, nodata, *, bands=BANDS, plants=None): | ||
| """The `DecodedRaster` a pixel-preserving setter must return on the | ||
| standard grid: the seeded pixels unchanged, with `nodata` per band.""" | ||
| return DecodedRaster( | ||
| random_raster_data( | ||
| dtype, bands=bands, height=HEIGHT, width=WIDTH, plants=plants | ||
| ), | ||
| nodata=nodata, | ||
| bbox=BBOX, | ||
| ) |
Member
There was a problem hiding this comment.
Is this the best place for this? Are the bands / bbox duplicated from the default for random_raster_data()? Maybe DecodedRaster.random() is better?
Contributor
Author
There was a problem hiding this comment.
Ill consider this as a followup in #1219
main's trinary override semantics (apache#1198) changed SedonaDB's RS_SetBandNoDataValue(rast, band, NULL) from propagating the NULL to clearing that band's nodata (probe: a band holding 7.0 comes back cleared while band 2 keeps its 7.0); Sedona Spark still returns a NULL raster. Split the combined NULL-args test: NULL band index still agrees (both NULL), NULL value is xfail-cataloged.
james-willis
commented
Sep 2, 2026
james-willis
marked this pull request as ready for review
September 2, 2026 19:42
james-willis
added a commit
to james-willis/sedona-db
that referenced
this pull request
Sep 2, 2026
Addresses the _anchor review comment on apache#1218: the standard random raster now has one definition. DecodedRaster.random builds it (seeded pixels on the RANDOM_GRID_* defaults, scalar nodata broadcast per band), DecodedRaster.write_geotiff writes it (GeoTIFF nodata is file-wide, so per-band anchors refuse to write), and write_random_geotiff and DBEngine.create_random_raster_view delegate to them. Anchors are the same object the fixture registers, so they cannot drift apart — the duplicated BANDS/HEIGHT/WIDTH/BBOX constants and the module-local _anchor helper in the parity suites go away.
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.
Stacked on #1211— #1211 has merged; main is merged in and the diff now shows only this PR's changes. One post-merge addition: main's trinary override semantics (#1198) changed SedonaDB'sRS_SetBandNoDataValue(rast, band, NULL)from propagating the NULL to clearing that band's nodata, while Sedona Spark still returns a NULL raster — the combined NULL-args test is split so NULL band index stays a passing agreement and NULL value is xfail-cataloged.Implements @paleolimbot's post-merge suggestion on #1203: an optional parameter on
compare()that lets the reference value be specified, guarding the parity claim against vacuous agreement — every engine returning the same wrong thing passes a pure engine-vs-engine comparison.The API
expectedis keyword-only and optional. The subject is asserted against the anchor first (clearest failure when all engines agree on a wrong value), then against each reference as before — so the references are anchored transitively. Parity-only remains the default for cases where the reference is hard to state.Coverage added
Anchors on every test whose expected value is trivially known:
test_rs_band_nodata: the written nodata for both bands, and[(None,)]for the no-nodata fixture.test_rs_setbandnodata,_band2,_overwrite,_two_arg_single_band): the fullDecodedRaster— the exact seeded pixels (reconstructible becauserandom_raster_datais seeded), the grid (stated explicitly so the anchor and the fixture can't drift apart), and the per-band nodata.The anchored tests place their grid with
bbox=, and the anchors state the same BBOX viaDecodedRaster's bbox construction (added to #1211) — the instance still carries the derivedgdal_transform, which is whatassert_decoded_equalcompares.The setter anchors close a real hole: with parity only, both engines no-opping identically (source nodata unchanged) would have passed every setter test.
Verification
21 passed, 14 xfailedlocally (pyspark 4.0.4, Sedona 1.9.1) after merging main, plustest_raster_testing.py(5 passed) in-branch.AssertionError; the correct anchors pass.This suite is not yet wired into CI (#1206 does that), so the counts above are from a local run.