Repository navigation
Expr simplifier doesn't simplify exprs that are same if you swap lhs with rhs regardless of cycles #14943
Description
Describe the bug
I was doing some improvements to merge in delta-rs to add an expr simplifier on our early pruning filter. While doing this I noticed that these type of expressions: s.foo = 'a' and 'a' = s.foo are not getting simplified into s.foo='a' directly but rather in s.foo='a' and s.foo='a' even when you set max cycles to ludicrous number of 100.
If the lhs and rhs can be swapped and then it's equal to an existing expression, it should be removed in one cicle.
See one of our logs after using the simplifier:
Before simplifying
[crates/core/src/operations/merge/mod.rs:864:5] commit_predicate.clone() = Some(
"unique_row_hash BETWEEN 'new_hash' AND 'new_hash' AND 202502 = month_id AND month_id = 202502 AND 20250226 = date_id AND date_id = 20250226",
)
After simplifying once with max cycles 100
[crates/core/src/operations/merge/mod.rs:886:5] commit_predicate.clone() = Some(
"unique_row_hash >= 'new_hash' AND unique_row_hash <= 'new_hash' AND month_id = 202502 AND month_id = 202502 AND date_id = 20250226 AND date_id = 20250226",
)
To Reproduce
This is the starting expression.
Some(
BinaryExpr(
BinaryExpr {
left: BinaryExpr(
BinaryExpr {
left: BinaryExpr(
BinaryExpr {
left: Column(
Column {
relation: Some(
Bare {
table: "t",
},
),
name: "unique_row_hash",
},
),
op: GtEq,
right: Literal(
Utf8("new_hash"),
),
},
),
op: And,
right: BinaryExpr(
BinaryExpr {
left: Column(
Column {
relation: Some(
Bare {
table: "t",
},
),
name: "unique_row_hash",
},
),
op: LtEq,
right: Literal(
Utf8("new_hash"),
),
},
),
},
),
op: And,
right: BinaryExpr(
BinaryExpr {
left: BinaryExpr(
BinaryExpr {
left: BinaryExpr(
BinaryExpr {
left: BinaryExpr(
BinaryExpr {
left: Column(
Column {
relation: Some(
Bare {
table: "t",
},
),
name: "month_id",
},
),
op: Eq,
right: Literal(
Int32(202502),
),
},
),
op: And,
right: BinaryExpr(
BinaryExpr {
left: Column(
Column {
relation: Some(
Bare {
table: "t",
},
),
name: "month_id",
},
),
op: Eq,
right: Literal(
Int64(202502),
),
},
),
},
),
op: And,
right: BinaryExpr(
BinaryExpr {
left: Column(
Column {
relation: Some(
Bare {
table: "t",
},
),
name: "date_id",
},
),
op: Eq,
right: Literal(
Int32(20250226),
),
},
),
},
),
op: And,
right: BinaryExpr(
BinaryExpr {
left: Column(
Column {
relation: Some(
Bare {
table: "t",
},
),
name: "date_id",
},
),
op: Eq,
right: Literal(
Int64(20250226),
),
},
),
},
),
},
),
)
Expected behavior
Simplify further
Additional context
No response
Activity
alamb commentedon Feb 28, 2025
I think this would be a nice improvement
There are code and existing tests that can simplify this type of expression. See tests here
datafusion/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
Line 1993 in 5e49094
Maybe we need to improve the logic somehow
This would be a good thing for someone to look into
ion-elgreco commentedon Feb 28, 2025
@alamb something to note, even after manually applying the optimizer again it doesn't simply
foo = 5 and foo = 5jayzhan211 commentedon Mar 4, 2025
We simplify A = B and B = A in
common_sub_expression_eliminatebut notsimplify_expressions@ion-elgreco You might want to add
CommonSubexprEliminateto your analyzer rulesalamb commentedon Mar 4, 2025
@jayzhan211 has a PR to fix this here:
alamb commentedon Mar 4, 2025
I wonder if this is another example of code that would be beneficial to pull into simplify_expressions 🤔
ion-elgreco commentedon Mar 4, 2025
If the fix gets included in DF46, then we can give it a spin in delta-rs to see if it fully resolves the issue
alamb commentedon Mar 4, 2025
I think it will not be present in DataFusion 46 (we have the RC out for voting now)
ion-elgreco commentedon Mar 5, 2025
Ah pity!
fix: dedup commutative AND/OR operands when canonicalize is disabled
fix: dedup commutative AND/OR operands when canonicalize is disabled
fix: dedup commutative AND/OR operands when canonicalize is disabled
fix: dedup commutative AND/OR operands when canonicalize is disabled (a…