fix: preserve null_aware on logical JoinNode proto round-trip#22104
fix: preserve null_aware on logical JoinNode proto round-trip#22104mithuncy wants to merge 4 commits into
Conversation
…#22065) Add `null_aware` to `JoinNode` (tag 9) and plumb it through the logical encoder/decoder. Without this, NOT IN semantics silently degrade to plain LeftAnti after `to_proto` -> `from_proto`.
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
neilconway
left a comment
There was a problem hiding this comment.
Overall this is a well-done PR! Nice work, @mithuncy
It does make me a bit concerned that an issue like this surfaced. It would be great if we had a systematic way to test that query plans roundtrip through proto, like we do for substrait (datafusion/sqllogictest/src/engines/datafusion_substrait_roundtrip_engine). That could be worth taking on as a follow-up.
| }; | ||
|
|
||
| builder.build() | ||
| // The builder paths above hardcode `null_aware = false`, so |
There was a problem hiding this comment.
Rather than first constructing the join with an incorrect null_aware field and then overwriting it here, can we try ensuring that the null_aware field is correct when the join is constructed? That would be more robust and avoids the intermediate period where we've constructed a value but it is in an incorrect state.
i.e., something like https://gist.github.com/neilconway/2dc2449dcf4cca3a60e1861e445e042c
There was a problem hiding this comment.
@neilconway Thanks a lot — much cleaner than mine, and it also fixes the null_equality drop on the same path.
Edited the now-obsolete null_equality related paragraph from the PR description. and LGTM
Per @neilconway's review: rather than building the Join through LogicalPlanBuilder (which hardcodes null_aware = false) and then overwriting the field, construct it directly via Join::try_new. Also reads null_equality from the proto, fixing the same shape of bug on the same decoder path. Drops the unused JoinConstraint import and adds a length-match check on left/right join keys (previously enforced by the builder).
Collapses a few unnecessarily-wrapped lines flagged by `cargo fmt --check` on CI.
Summary
Closes #22065.
null_awarewas missing fromJoinNodein the logical proto(it was added to the physical
HashJoinExecNodein #19635). Theencoder dropped it via
..destructuring and the decoder had nofield to restore it from, so any
to_proto->from_protoroundtrip silently downgraded a null-aware LeftAnti (NOT IN semantics)
to a plain LeftAnti and returned wrong rows.
Changes
bool null_aware = 9;toJoinNode(backward compatible).null_awarethrough; decoder switches toJoin::try_newso the field is set at construction (also fixesnull_equality, same bug on the same path).Test plan
cargo test -p datafusion-proto --test proto_integration cases::roundtrip_logical_planpasses (incl. newroundtrip_join_null_aware).