andygrove commented on PR #5806:
URL:
https://github.com/apache/datafusion-comet/pull/5806#issuecomment-5607870344
The null-map validity concern is fixed in 7d527c148, and your counterexample
was exact.
The gather validity only recorded whether a match existed, so it never
consulted the map's own null buffer. The kernel now checks it before the mask
scan and skips a null row's entries entirely, which is also marginally cheaper
than scanning them and discarding the result.
I built your shape and confirmed it reads the wrong value before the fix
rather than trusting the reading. `offsets [0, 1, 3]`, validity `[false,
true]`, physical entries `a -> 7`, `a -> 1`, `b -> 2`, so row 0 is NULL but
still spans a live `a -> 7`:
```
null_map_row_reads_null_even_with_live_entries ... FAILED
left: [Some(7), Some(1)]
right: [None, Some(1)]
null_map_row_reads_null_with_a_per_row_lookup_key ... FAILED
left: [Some(7), Some(2)]
right: [None, Some(2)]
```
Both pass with the fix, and all 24 `map_funcs` tests pass. `cargo clippy -p
datafusion-comet-spark-expr --all-targets -- -D warnings` and `cargo fmt --all
-- --check` are clean.
I covered both key paths deliberately, because they fail for slightly
different reasons. The constant-key path compares the null row's entry against
the broadcast key. The per-row path gathers a key per entry, so the null row's
entry is compared against that row's own lookup key, and the mask is set there
too. Masking at the gather-index step covers both, which is why I put it there
rather than in `key_match_mask`.
On why there is no Scala test: this shape is not reachable from Spark SQL.
Spark's own map builders give a null row an empty offset range, so an
end-to-end `NULL` map already read `NULL` before this change, and the existing
suite covers that. What produces a null row over retained entries is a null
mask added above intact child buffers inside Comet, which is why the guard
belongs at the kernel boundary where every caller gets the same contract, and
why the regression has to be built at that boundary too. If you would rather I
also asserted the reachable case end to end, the existing
`constant_key_hit_and_miss` row 3 and the Scala suite's whole-map-null cases
already do.
I left the wider `map_extract` gap in pinned DataFusion 55 alone, as you
noted it is pre-existing there. Worth an upstream issue, but not something this
PR should carry.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
