fix: preserve null_aware on logical JoinNode proto round-trip by mithuncy · Pull Request #22104 · apache/datafusion

mithuncy · 2026-05-11T13:04:55Z

Summary

Closes #22065.

null_aware was missing from JoinNode in the logical proto
(it was added to the physical HashJoinExecNode in #19635). The
encoder dropped it via .. destructuring and the decoder had no
field to restore it from, so any to_proto -> from_proto round
trip silently downgraded a null-aware LeftAnti (NOT IN semantics)
to a plain LeftAnti and returned wrong rows.

Changes

Proto: add bool null_aware = 9; to JoinNode (backward compatible).
Encoder/decoder plumb null_aware through; decoder switches to
Join::try_new so the field is set at construction (also fixes
null_equality, same bug on the same path).
Regression test for NOT-IN-with-NULL round trip.

Test plan

cargo test -p datafusion-proto --test proto_integration cases::roundtrip_logical_plan passes (incl. new roundtrip_join_null_aware).
Clippy clean.

…#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`.

github-actions · 2026-05-11T20:15:00Z

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

     Cloning apache/main
    Building datafusion-proto v53.1.0 (current)
       Built [  58.542s] (current)
     Parsing datafusion-proto v53.1.0 (current)
      Parsed [   0.133s] (current)
    Building datafusion-proto v53.1.0 (baseline)
       Built [  56.088s] (baseline)
     Parsing datafusion-proto v53.1.0 (baseline)
      Parsed [   0.138s] (baseline)
    Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   1.611s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field JoinNode.null_aware in /home/runner/work/datafusion/datafusion/datafusion/proto/src/generated/prost.rs:392
  field JoinNode.null_aware in /home/runner/work/datafusion/datafusion/datafusion/proto/src/generated/prost.rs:392

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [ 118.590s] datafusion-proto

neilconway

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.

@neilconway

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.

Verified to fail on the un-patched decoder and pass with the fix.

alamb · 2026-05-12T15:00:50Z

+
+// Regression test for null_equality round-trip (related to #22065).
+#[tokio::test]
+async fn roundtrip_join_null_equality() -> Result<()> {


I don't fully undersand why both tests are needed here

The first test exercises null_aware = true (via NOT IN SQL); the second forces null_equality = NullEqualsNull. They both use the round-trip Debug compare, but each puts one field into a non-default state, a decoder that drops the field to its default. The first test uses the default null_equality (NullEqualsNothing), so a regression of that exact shape on null_equality would slip past it.
Happy to consolidate into a single test that exercises both non-default fields on the same Join if you'd prefer — just wanted to flag the coverage tradeoff before deleting either one

maybe worth a quick comment to highight the differecne

Thanks each test comment now spells out the non-default field and the one held at default.

@alamb

Per @alamb's review: each test comment now spells out which field is at a non-default value and which is at the default, so the coverage split is obvious without cross-referencing.

fix: preserve null_aware on logical JoinNode proto round-trip (apache…

4e78b81

…#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`.

github-actions Bot added the proto Related to proto crate label May 11, 2026

github-actions Bot added the auto detected api change Auto detected API change label May 11, 2026

neilconway reviewed May 11, 2026

View reviewed changes

Comment thread datafusion/proto/src/logical_plan/mod.rs Outdated

mithuncy added 3 commits May 12, 2026 02:48

Merge branch 'main' into fix/22065-logical-join-null-aware

7ce687a

style: cargo fmt

3982ebe

Collapses a few unnecessarily-wrapped lines flagged by `cargo fmt --check` on CI.

kumarUjjawal reviewed May 12, 2026

View reviewed changes

Comment thread datafusion/proto/tests/cases/roundtrip_logical_plan.rs

mithuncy added 2 commits May 12, 2026 12:12

test: add null_equality round-trip regression test (apache#22065)

a64d469

Verified to fail on the un-patched decoder and pass with the fix.

Merge branch 'main' into fix/22065-logical-join-null-aware

b07b648

alamb reviewed May 12, 2026

View reviewed changes

mithuncy added 2 commits May 12, 2026 20:52

Merge branch 'main' into fix/22065-logical-join-null-aware

0b6cb71

test: clarify what each join round-trip test exercises

54a448f

Per @alamb's review: each test comment now spells out which field is at a non-default value and which is at the default, so the coverage split is obvious without cross-referencing.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: preserve null_aware on logical JoinNode proto round-trip#22104

fix: preserve null_aware on logical JoinNode proto round-trip#22104
mithuncy wants to merge 8 commits into
apache:mainfrom
mithuncy:fix/22065-logical-join-null-aware

mithuncy commented May 11, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 11, 2026 •

edited

Loading

Uh oh!

neilconway left a comment

Uh oh!

Uh oh!

Uh oh!

alamb May 12, 2026

Uh oh!

mithuncy May 12, 2026 •

edited

Loading

Uh oh!

alamb May 12, 2026

Uh oh!

mithuncy May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mithuncy commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Uh oh!

github-actions Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neilconway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb May 12, 2026

Choose a reason for hiding this comment

Uh oh!

mithuncy May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb May 12, 2026

Choose a reason for hiding this comment

Uh oh!

mithuncy May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mithuncy commented May 11, 2026 •

edited

Loading

github-actions Bot commented May 11, 2026 •

edited

Loading

mithuncy May 12, 2026 •

edited

Loading