fix: Derive Substrait intersection nullability from every input - #25091
Open
namanjain24-sudo wants to merge 1 commit into
Open
fix: Derive Substrait intersection nullability from every input#25091namanjain24-sudo wants to merge 1 commit into
namanjain24-sudo wants to merge 1 commit into
Conversation
The Substrait consumer derived all three intersection schemas from the primary input alone, so a field that the intersection makes required stayed nullable in the logical output schema. Narrow an intersection's nullability to `left AND right` per field. The left semi join it compiles to matches nulls with nulls, so a field is nullable in the result only when both inputs make it nullable, which reproduces the spec's rule for the multiset intersections and, because the right side is the union of the secondary inputs, for the primary intersection as well. Closes apache#25042.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
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_PRIMARYit is nullable only when it is nullable in the primaryinput and in at least one secondary input.
from_set_relbuilds intersections withLogicalPlanBuilder::intersect, whichcompiles to a left semi join and therefore keeps the left input's nullability.
What changes are included in this PR?
Intersections now go through a small helper that narrows the result's
nullability to
left AND rightper field.The join matches nulls with nulls (
NullEquality::NullEqualsNull) on everyfield, 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:
input requires it;
INTERSECTION_PRIMARYthe right side is the union of the secondaryinputs, 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
MINUSoperations are notaffected.
What is the testing strategy for this PR?
New test
intersect_nullabilityindatafusion/substrait/tests/cases/logical_plans.rs,with three plans added under
tests/testdata/test_plans/. They intersect threetables carrying the same four columns with the spec's nullability pattern
(
?marks nullable):INTERSECTION_PRIMARYa, b?, c?, d?INTERSECTION_MULTISETa, b, c, d?INTERSECTION_MULTISET_ALLa, b, c, d?The test fails on
main(a?whereais expected) and passes here. It alsoexecutes 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 nowmatch the expected column, and the union and primary-minus controls are
unchanged:
The existing
datafusion-substraitsuite passes unchanged, including theintersection 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, sinceLogicalPlanBuilder::intersectkeeps the left nullability for every caller. Ikept 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.