berkaysynnada commented on code in PR #13805:
URL: https://github.com/apache/datafusion/pull/13805#discussion_r1887986662
##########
datafusion/physical-expr/src/equivalence/properties.rs:
##########
@@ -1828,22 +1831,34 @@ fn calculate_union_binary(
}
// First, calculate valid constants for the union. An expression is
constant
- // at the output of the union if it is constant in both sides.
- let constants: Vec<_> = lhs
+ // at the output of the union if it is constant in both sides with
matching values.
+ let constants = lhs
.constants()
.iter()
- .filter(|const_expr| const_exprs_contains(rhs.constants(),
const_expr.expr()))
- .map(|const_expr| {
- // TODO: When both sides have a constant column, and the actual
- // constant value is the same, then the output properties could
- // reflect the constant is valid across all partitions. However we
- // don't track the actual value that the ConstExpr takes on, so we
- // can't determine that yet
-
ConstExpr::new(Arc::clone(const_expr.expr())).with_across_partitions(false)
+ .filter_map(|lhs_const| {
+ // Find matching constant expression in RHS
+ rhs.constants()
+ .iter()
+ .find(|rhs_const| rhs_const.expr().eq(lhs_const.expr()))
+ .map(|rhs_const| {
+ let mut const_expr =
ConstExpr::new(Arc::clone(lhs_const.expr()));
+
+ // If both sides have matching constant values, preserve
the value and set across_partitions=true
+ if let (Some(lhs_val), Some(rhs_val)) =
+ (lhs_const.value(), rhs_const.value())
+ {
+ if lhs_val == rhs_val {
+ const_expr = const_expr
+ .with_across_partitions(true)
+ .with_value(lhs_val.clone());
+ }
+ }
+ const_expr
+ })
})
- .collect();
+ .collect::<Vec<_>>();
- // remove any constants that are shared in both outputs (avoid double
counting them)
+ // Remove any constants that are shared in both outputs (avoid double
counting them)
for c in &constants {
lhs = lhs.remove_constant(c);
rhs = rhs.remove_constant(c);
Review Comment:
When I remove this for loop, the tests don't fail. Can you check if they are
really needed? If yes, can we write a test for that scenario also in this PR?
--
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]
