namanjain24-sudo opened a new pull request, #25091: URL: https://github.com/apache/datafusion/pull/25091
## Which issue does this PR close? - Closes #25042. ## Rationale for this change The Substrait consumer derived all three intersection schemas from the primary input alone, so a field the intersection makes required stayed nullable in the logical output schema. The [Set Operation rules] give intersections a different rule: for the multiset intersections a field is required as soon as any input requires it, and for `INTERSECTION_PRIMARY` it is nullable only when it is nullable in the primary input and in at least one secondary input. `from_set_rel` builds intersections with `LogicalPlanBuilder::intersect`, which compiles to a left semi join and therefore keeps the left input's nullability. [Set Operation rules]: https://substrait.io/relations/logical_relations/#set-operation ## What changes are included in this PR? Intersections now go through a small helper that narrows the result's nullability to `left AND right` per field. The join matches nulls with nulls (`NullEquality::NullEqualsNull`) on every field, so a left row holding a null in some field only survives when the right input holds a null there too — a field is nullable in the result only when both inputs make it nullable. Applied to each step, that single rule reproduces both spec rules: - the multiset intersections chain pairwise, so the result is required when any input requires it; - for `INTERSECTION_PRIMARY` the right side is the union of the secondary inputs, whose field is nullable exactly when some secondary makes it nullable, which yields "nullable in the primary and in at least one secondary". When nothing needs narrowing the plan is returned unchanged, so the common all-nullable case is untouched. Unions and the `MINUS` operations are not affected. ## What is the testing strategy for this PR? New test `intersect_nullability` in `datafusion/substrait/tests/cases/logical_plans.rs`, with three plans added under `tests/testdata/test_plans/`. They intersect three tables carrying the same four columns with the spec's nullability pattern (`?` marks nullable): ``` primary a? b? c? d? secondary a b c? d? secondary a b? c d? ``` | Operation | Result | | --- | --- | | `INTERSECTION_PRIMARY` | `a, b?, c?, d?` | | `INTERSECTION_MULTISET` | `a, b, c, d?` | | `INTERSECTION_MULTISET_ALL` | `a, b, c, d?` | The test fails on `main` (`a?` where `a` is expected) and passes here. It also executes each plan, so the narrowed schema is checked to survive optimization and execution. I also ran the probe from the issue. The three `setop_intersection_*` lines now match the expected column, and the union and primary-minus controls are unchanged: ``` setop_intersection_primary [c0, c1, c2, c3, c4, c5?, c6?, c7?] setop_intersection_multiset [c0, c1, c2, c3, c4, c5, c6, c7?] setop_intersection_multiset_all [c0, c1, c2, c3, c4, c5, c6, c7?] ``` The existing `datafusion-substrait` suite passes unchanged, including the intersection roundtrip tests. ## Are there any user-facing changes? Intersections consumed from Substrait now report a narrower, spec-conforming nullability. No public API changes. One note for reviewers: the same narrowing would apply to SQL `INTERSECT`, since `LogicalPlanBuilder::intersect` keeps the left nullability for every caller. I kept this change inside the Substrait consumer to match the scope of the issue and to avoid changing SQL plans in the same PR. If you would rather see the rule live in `LogicalPlanBuilder`, I am happy to move it. -- 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]
