kosiew commented on code in PR #25091:
URL: https://github.com/apache/datafusion/pull/25091#discussion_r4059907859
##########
datafusion/substrait/src/logical_plan/consumer/rel/set_rel.rs:
##########
@@ -77,16 +79,101 @@ async fn intersect_rels(
let mut rel = consumer.consume_rel(&rels[0]).await?;
for input in &rels[1..] {
- rel = LogicalPlanBuilder::intersect(
- rel,
- consumer.consume_rel(input).await?,
- is_all,
- )?;
+ rel = intersect_rel(rel, consumer.consume_rel(input).await?, is_all)?;
}
Ok(rel)
}
+/// Intersects two relations, giving the result the nullability the Substrait
+/// [Set Operation rules] prescribe.
+///
+/// [`LogicalPlanBuilder::intersect`] compiles an intersection into a left semi
+/// join, so on its own the result keeps the left input's nullability. The join
+/// matches nulls with nulls, so a left row holding a null in some field only
+/// survives when the right input holds a null there too. A field is therefore
+/// nullable in the result only when it is nullable in *both* inputs.
+///
+/// Applied to each step of a chain, that gives the spec's rule for the
multiset
+/// intersections - a field 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 input makes it
nullable,
+/// so the same rule yields "nullable in the primary input and in at least one
+/// secondary input".
+///
+/// When the right input requires a field the left input leaves nullable, the
+/// intersection is built as an inner join against the distinct right rows
+/// instead, and that field is read from the right side. Matched rows hold
equal
+/// values, so the result is unchanged, and the field is non-nullable because
+/// its source is: the logical and the physical planner both derive that from
+/// the input schema, so the plan, the physical plan and the batches agree.
+/// Joining against distinct right rows keeps each left row at most once, as
the
+/// semi join does.
+///
+/// [Set Operation rules]:
https://substrait.io/relations/logical_relations/#set-operation
+fn intersect_rel(
+ left: LogicalPlan,
+ right: LogicalPlan,
+ is_all: bool,
+) -> datafusion::common::Result<LogicalPlan> {
+ let left_fields = left.schema().fields();
+ let right_fields = right.schema().fields();
+ // Only a field that differs from its right counterpart in nullability
alone
+ // is read from the right side, so every other attribute stays the left's.
+ let from_right: Vec<bool> = left_fields
+ .iter()
+ .zip(right_fields.iter())
+ .map(|(left, right)| {
+ left.is_nullable()
+ && !right.is_nullable()
+ && left.data_type() == right.data_type()
+ && left.metadata() == right.metadata()
+ })
+ .collect();
+
+ // `intersect` also reports inputs of different widths. The join would
merge
+ // the right input's schema metadata into the result, so that must match
too.
+ if left_fields.len() != right_fields.len()
+ || left.schema().metadata() != right.schema().metadata()
Review Comment:
I think this fallback still leaves the original bug reachable. If the schema
metadata differs, we fall back to `LogicalPlanBuilder::intersect`, which uses
the left-semi plan and keeps the nullable left-side field.
`ensure_schema_compatibility` only checks type and nullability compatibility,
so differing metadata can still be valid here.
Could we keep using the narrowing path and explicitly preserve the left-side
metadata when projecting a required column from the right? The alias API
supports attaching metadata, so that should let us retain the left schema
metadata without giving up the nullability fix.
The same issue can happen with field metadata: if a nullable left field and
required right field differ only in metadata, that field will not be selected
from the right unless some other field happens to trigger this path.
It would be useful to add a regression test with differing schema and field
metadata and assert the exact logical, physical, and collected-batch schemas.
--
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]
