jayzhan211 commented on code in PR #27288:
URL: https://github.com/apache/datafusion/pull/27288#discussion_r4572781324
##########
datafusion/datasource/src/file_scan_config/mod.rs:
##########
@@ -1384,23 +1384,39 @@ impl FileScanConfig {
eq_properties: &mut EquivalenceProperties,
schema: &Schema,
) -> Result<()> {
- // Gather valid equality pairs from the filter expression
- let equal_pairs =
split_conjunction(filter).into_iter().filter_map(|expr| {
+ let input_eq_properties = eq_properties.clone();
+ let mut constants = vec![];
+ for expr in split_conjunction(filter) {
// Ignore any binary expressions that reference non-existent
columns in the current schema
// (e.g. due to unnecessary projections being removed)
- reassign_expr_columns(Arc::clone(expr), schema)
- .ok()
- .and_then(|expr| match expr.downcast_ref::<BinaryExpr>() {
- Some(expr) if expr.op() == &Operator::Eq => {
- Some((Arc::clone(expr.left()),
Arc::clone(expr.right())))
- }
- _ => None,
- })
- });
-
- for (lhs, rhs) in equal_pairs {
- eq_properties.add_equal_conditions(lhs, rhs)?
+ let Ok(expr) = reassign_expr_columns(Arc::clone(expr), schema)
else {
+ continue;
+ };
+ let Some(binary) = expr.downcast_ref::<BinaryExpr>() else {
+ continue;
+ };
+ if binary.op() != &Operator::Eq {
+ continue;
+ }
+ // Follow `FilterExec`: only an equality with a column on one side
Review Comment:
Agree with @adriangb. I think the simplest way to avoid two diverging copies
is to drop this one: #26967 (approved) fixes the root cause in
`EquivalenceGroup`, and with only #26967 on this PR's base, the #27287 query
returns `y 1` and your new slt test passes unchanged.
Keeping it also costs a sort: with `pushdown_filters = true`, equalities
with no column on either side no longer form a class at the scan, so `WHERE a =
'x' AND upper(a) = upper(b) ORDER BY upper(b)` gains a `SortExec` that main
doesn't need. Could you drop the `mod.rs` change, keep the slt test, and land
it after #26967?
<details>
<summary>Plans that gain a sort with this PR (all have none, or fewer keys,
on main and on main + #26967)</summary>
```sql
SET datafusion.execution.parquet.pushdown_filters = true;
-- t: parquet with VARCHAR columns a, b
EXPLAIN SELECT a, b FROM t WHERE a = 'x' AND upper(a) = upper(b) ORDER BY
upper(b); -- SortExec added
EXPLAIN SELECT a, b FROM t WHERE upper(a) = 'X' AND upper(a) = upper(b)
ORDER BY upper(b); -- SortExec + SPM added
EXPLAIN SELECT a, b FROM t WHERE upper(a) = upper(b) ORDER BY upper(a),
upper(b); -- sorts on 2 keys instead of 1
```
</details>
--
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]
