test(integration): broaden nodata function parity coverage - #1203
Conversation
Extend the SedonaDB vs Sedona Spark parity suite for RS_BandNoDataValue and RS_SetBandNoDataValue: all eight GDAL-writable band dtypes, NaN and dtype-unrepresentable nodata values, out-of-range band indexes, NULL argument propagation, the 2-arg and 4-arg setter forms, overwrite and idempotent re-set, and nodata sentinels planted into the pixels. Known divergences are cataloged as xfails with the observed behavior of each engine in the reason.
Both test modules carried an identical copy of the helper and the fixture grid constants; they now import them from a shared sibling module.
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Not mandatory, but I tend to like the style of writing a test that asserts a hard-coded result (like the postgis integration tests) as a sort of secondary guard on accidental meaningless assertions...I think we've gotten the Raster constructor to a point where eng.assert_query_result("sql stuff", reference) could work for these.
I have a few comments trying to avoid too many test helpers scattered in too many places, but your call (this integration harness is better than no integration harness!).
Did you want to run these in CI? / Are they running in CI now?
| def _outcome(engine, sql): | ||
| """`sql`'s result on `engine`, normalized so outcomes compare with `==`: | ||
| `("value", x)` with a NaN result replaced by the string "NaN" (bare `==` | ||
| fails NaN == NaN even when the engines agree), or `("error",)` when | ||
| execution raises — the engines' error types and messages are incomparable, | ||
| so error parity is parity on refusal.""" | ||
| try: | ||
| value = _one(engine, sql) | ||
| except Exception: | ||
| return ("error",) | ||
| if isinstance(value, float) and math.isnan(value): | ||
| return ("value", "NaN") | ||
| return ("value", value) |
There was a problem hiding this comment.
This is a squirrely way to stick these types of comparisons...there are more straightforward ways to assert that the expected result is NaN or that an error should occur in both cases. If we need helpers for either of those, you have a setup that can handle utilities (and test them if needed). Putting them in a common file also lets future developers find them more easily instead of repeating them.
For example, in SedonaDB's testing this might be eng.assert_query_result("SELECT isnan(foofy)", True) or eng.assert_query_result("SELECT the_raster", expected_raster). You'd have to modify the query result checker to handle raster equality but that will help you write other tests, too. This could be a thin wrapper around numpy's equality assertion.
…lain comparisons Move the write-and-register helper onto DBEngine as create_random_raster_view (next to create_raster_view, mirroring the geoparquet view methods) and delete parity.py; write_random_geotiff gains a plants passthrough so the pixel-planting tests need no custom writer. Drop the _outcome normalization: divergence tests now compare engine results directly (a raised error is what trips the xfail), and the NaN tests ask the question through SQL isnan so the comparison is a plain boolean.
I am ambivalent on this. It would be good coverage but add complexity to the tests. I feel it is somewhat a mixing of concerns.
Looking
Probably will but not in this PR. I wanted to let this bake a little bit before doing so. |
Each scalar test now asserts SedonaDB's result against Sedona Spark's live output via sedona.assert_query_result(sql, _tuples(spark, sql)). result_to_tuples is the cross-engine form the checker already documents: Arrow stringifies values identically on both sides, NULL stays None, and NaN becomes the string 'nan' — so the isnan detour and any bespoke comparison helpers are gone.
|
moving to draft as I screw around with how to perform assertions |
…assertion One entry point in sedonadb.testing for the cross-engine parity claim: the first engine is the subject, every further engine an expected side. Table results compare in result_to_tuples form; a sedona.raster result dispatches to decode_raster_result + assert_decoded_equal, with all engines agreeing on a NULL raster accepted. Every non-error parity test in both suite modules is now compare(sql, sedona, spark).
compare() previously sniffed the subject's collected Arrow schema for the sedona.raster extension, which silently assumed the subject was SedonaDB. The dispatch now asks a new DBEngine.result_has_raster(sql) hook answered from each engine's lazy result schema — SedonaDB via the Arrow C schema of the unexecuted plan, Sedona Spark via RasterType in the analyzed DataFrame schema — so either engine can be the subject and nothing executes just to pick the comparison path.
|
merging this with some small tweaks after @paleolimbot 's review. LMK if you have issues and I will raise a follow up |
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Sounds good to me.
I still think the ability to assert that two rasters are equal (and assert an exact result when it's easy to compute) will result in fewer accidental bogus tests (e.g., our recent discovery that some spatial bench results were just the table repeated three times and sorted, because nobody looked at the result). You could add an optional parameter to compare() that lets the reference value be specified (sometimes this is hard, but sometimes it is not).
| bands=2, | ||
| height=6, | ||
| width=7, | ||
| gdal_transform=(100.0, 2.0, 0.0, 500.0, 0.0, -3.0), |
There was a problem hiding this comment.
Can this be a bbox instead of a transform? Or can you allow the option of a bbox and use it for plausibly readable tests?
2 participants
Follow-up to #1159: broaden the parity coverage for the two functions the suite already exercises,
RS_BandNoDataValueandRS_SetBandNoDataValue. The suite grows from 6 tests to 34 (21 pass, 13 xfail).Every case was first probed against both engines (SedonaDB at this branch, Sedona Spark 1.9.1 on pyspark 4.0.4), so each xfail reason states the observed behavior of each engine rather than a guess.
New coverage that passes (the engines agree)
RS_BandNoDataValueacross all eight GDAL-writable band dtypes (adds int8, uint16, int16, uint32, float32)replaceform (Sedona's Java layer ships the overload, but the 1.9.1 SQL binding fails to evaluate it)Divergences cataloged as xfails
rs_band_accessors.rs)unwrap_or(1))CASEPer the suite's convention these xfails double as the fix catalog — each flips to xpass when the gap closes on either side. Which side should move is left open per case (several SedonaDB behaviors are documented as deliberate).
This suite is not wired into CI; the counts above are from a local run (
21 passed, 13 xfailed in 13son a warm JVM).