Copilot commented on code in PR #26059:
URL: https://github.com/apache/datafusion/pull/26059#discussion_r4264486605
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -170,6 +174,23 @@ impl OptimizerRule for EliminateCrossJoin {
left = rewrite_children(self, left, config)?.data;
+ // Keep any original ON predicates that could not be assigned to one
+ // of the rebuilt joins. In particular, an equijoin key can reference
+ // columns from more than one flattened input on the same side, so it
+ // may only become evaluable after the join graph has been rebuilt.
+ for (left_key, right_key, original_null_equality) in
original_join_keys {
+ if !all_join_keys.contains(&left_key, &right_key) {
+ all_filters.push(binary_expr(
+ left_key.clone(),
+ match original_null_equality {
+ NullEquality::NullEqualsNothing => Operator::Eq,
+ NullEquality::NullEqualsNull =>
Operator::IsNotDistinctFrom,
+ },
+ right_key.clone(),
+ ));
+ }
+ }
Review Comment:
`all_join_keys.contains(&left_key, &right_key)` may be direction-sensitive.
If a rebuilt join ends up with the equijoin expressed as `(right, left)`
(common if join inputs are reordered/swapped), this check can return false and
incorrectly add a redundant residual predicate. Consider making the containment
check order-insensitive here (e.g., also check `contains(&right_key,
&left_key)`), or provide an order-insensitive `contains_pair` API on
`JoinKeySet` and use that instead.
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -1452,6 +1502,46 @@ mod tests {
Ok(())
}
+ #[test]
+ fn preserve_null_equality_for_unassigned_join_key() -> Result<()> {
+ let t1 = test_table_scan_with_name("t1")?;
+ let t2 = test_table_scan_with_name("t2")?;
+ let t3 = test_table_scan_with_name("t3")?;
+ let left = LogicalPlanBuilder::from(t1).cross_join(t2)?.build()?;
+ let join_schema = Arc::new(build_join_schema(
+ left.schema(),
+ t3.schema(),
+ &JoinType::Inner,
+ )?);
+
+ // The first key can be assigned to a rebuilt join, but the second
+ // spans two inputs on the left and must remain as a residual filter.
+ let plan = LogicalPlan::Join(Join {
+ left: Arc::new(left),
+ right: Arc::new(t3),
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![
+ (col("t1.a"), col("t3.a")),
+ (
+ binary_expr(col("t1.b"), Operator::Plus, col("t2.b")),
+ col("t3.c"),
+ ),
+ ],
+ filter: None,
+ schema: join_schema,
+ null_equality: NullEquality::NullEqualsNull,
+ null_aware: false,
+ null_aware_value_keys: 1,
+ });
+
+ let optimized = EliminateCrossJoin::new()
+ .rewrite(plan, &OptimizerContext::new())?
+ .data;
+ assert!(optimized.to_string().contains("IS NOT DISTINCT FROM"));
Review Comment:
This assertion is string-based and can become flaky with plan formatting
changes. Prefer asserting on the optimized plan structure (e.g., locating a
residual `Filter` expression containing `Operator::IsNotDistinctFrom`, or
checking the presence of a specific `Expr::BinaryExpr` in the expected
location) to make the test robust to formatting/pretty-print changes.
--
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]
