Re: [I] Expr formatting missing parentheses [datafusion]
jayendra13 commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-4033409765 Hi @AndreaBozzo, thanks for the heads-up! I think our #20851 actually avoids the wall(full SLT suit) you are talking about. #20206 uses unconditional wrapping — every child BinaryExpr gets parentheses. This changes schema names even when precedence already gives the correct reading, which is what triggers the optimize_projections mismatches. What I am doing at #20851 is only adding parentheses when the child operator has lower precedence than the parent, matching the logic already in the Display impl for BinaryExpr. So 1 + 2 + 3 stays unchanged (same precedence, no parens needed), while (1 + 2) * 3 correctly gets them. So far all 642 optimizer unit tests pass with only 2 snapshot updates. -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
AndreaBozzo commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-4033245625 Hi @jayendra13 , as is, your attempt will still hit the same walls as i did, but as i struggle to compile locally DF on my laptop, i'm happy to help you reviewing your pr if you pursue it. -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
jayendra13 commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-4032997609 Hi @AndreaBozzo, I've been working on the same issue and took a slightly different approach — precedence-based parenthesization instead of unconditional wrapping. The idea is to only add parentheses when the child operator has lower precedence than the parent, matching the logic already used by the Display impl for BinaryExpr (line 621). This avoids unnecessary parens for same/higher precedence cases, which may help with the SLT failures you're seeing from optimize_projections. For example, SELECT 1+2+3: - Before: Int64(1) + Int64(2) + Int64(3) (already correct, no parens needed) - Unconditional: (Int64(1) + Int64(2)) + Int64(3) (unnecessary parens, changes schema name) - Precedence-based: Int64(1) + Int64(2) + Int64(3) (unchanged, no schema mismatch) My branch is here: https://github.com/jayendra13/datafusion/tree/fix-schema-parens Happy to collaborate or consolidate — let me know what you think. -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
AndreaBozzo commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-3858652316 Yes, i'm up to re-attempt this -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
Jefffrey commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-3857375682 > I owe a correction on my previous comment. > > After re-investigating, the test failures in PR [#19916](https://github.com/apache/datafusion/pull/19916) were just snapshot mismatches, not actual optimizer breakage. The schema_name() keys are used within single optimization passes, so as long as the format change is consistent, field-matching works fine. > > I misread the failures during a frustrating debugging session (multiple crashes didn't help my judgment) and assumed the worst. The fix was actually correct, I just needed to update the test expectations instead of reverting. Would you be interested in reattempting the PR, implementing the fix for `SchemaDisplay` (and `SqlDisplay`)? I think then we would be good to close this issue -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
AndreaBozzo commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-3855289416 I owe a correction on my previous comment. After re-investigating, the test failures in PR #19916 were just snapshot mismatches, not actual optimizer breakage. The schema_name() keys are used within single optimization passes, so as long as the format change is consistent, field-matching works fine. I misread the failures during a frustrating debugging session (multiple crashes didn't help my judgment) and assumed the worst. The fix was actually correct, I just needed to update the test expectations instead of reverting. -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
Jefffrey commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-3850721213 Thanks @AndreaBozzo Would you be able to expand on this part? > However, `SchemaDisplay` output is also used as a **field-matching key by optimizers** (e.g. `common_subexpr_eliminate`, `push_down_filter`, `decorrelate`). These optimizers compare `schema_name()` strings to match fields across plan nodes. Changing the format of `SchemaDisplay` changes these keys, which breaks field resolution. Does this just mean we need to update the tests? Because if we change how `SchemaDisplay` is computed for binary expressions, this shouldn't affect optimizer key lookup since the column name should be consistent with the new 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]
Re: [I] Expr formatting missing parentheses [datafusion]
AndreaBozzo commented on issue #16054:
URL: https://github.com/apache/datafusion/issues/16054#issuecomment-3824269294
I worked on this in PR #19916 (now closed) and want to share the findings,
since the issue is more nuanced than it appears.
### The root problem: `SchemaDisplay` serves two conflicting purposes
The CLI column header for `select (1+2)*3` comes from this chain:
```
Expr::qualified_name()
→ Expr::schema_name()
→ SchemaDisplay(self).to_string()
→ used as Field name in DFSchema
→ propagated to physical plan schema
→ used by Arrow pretty printer as column header
```
`SchemaDisplay` for `BinaryExpr` currently has **no parenthesization at
all**:
```rust
Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
write!(f, "{} {op} {}", SchemaDisplay(left), SchemaDisplay(right))
}
```
This is why `(1+2)*3` displays as `Int64(1) + Int64(2) * Int64(3)` — it's
misleading.
However, `SchemaDisplay` output is also used as a **field-matching key by
optimizers** (e.g. `common_subexpr_eliminate`, `push_down_filter`,
`decorrelate`). These optimizers compare `schema_name()` strings to match
fields across plan nodes. Changing the format of `SchemaDisplay` changes these
keys, which breaks field resolution.
I hit this problem in #19916: adding parentheses to `SchemaDisplay` broke
optimizer tests, so I had to revert it. But without changing `SchemaDisplay`,
the CLI column headers can't be fixed — because that's where they come from.
### What's NOT broken
- **`Display` for `BinaryExpr`** already has correct precedence-based
parenthesization on main. `(1+2)*3` displays as `(Int64(1) + Int64(2)) *
Int64(3)`. This is used in `EXPLAIN` plans and debug logging.
- **`SqlDisplay`** has no parenthesization, but adding it (with precedence
logic) would be straightforward.
### What a proper fix would need
The core issue is that `SchemaDisplay` is **dual-purpose**: it's both a
user-facing column name and an internal optimizer field-matching key. These two
concerns need different formatting, but they're currently coupled through
`Expr::schema_name()`.
A proper fix would likely need to decouple these — for example by
introducing a separate `display_name()` for user-facing output while keeping
`schema_name()` stable for optimizer internals. This is a bigger architectural
change than just "add parentheses."
Wanted to document this here so future contributors don't hit the same wall.
--
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]
Re: [I] Expr formatting missing parentheses [datafusion]
JSOD11 commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-3465379440 Seems like leaving the parentheses will be fine, but there's still some work to do here. Happy to take a shot at this if no one is already working on it — let me know. -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
2010YOUY01 commented on issue #16054:
URL: https://github.com/apache/datafusion/issues/16054#issuecomment-2924301139
> I've had a closer look, we simply wrap `- {foo}` in parentheses regardless
of whether that's strictly necessary. Following the same principle for
`BinaryExpr` won't look as pretty as it could, but it would always be correct.
To avoid adding parentheses when not necessary, we'd have to pass more
information into `SchemaDisplay`. We'd need to be able to access the parent's
precedence to make a decision.
Thank you for the help. `DuckDB` also always add parentheses (e.g.
`(-(1+2))`), I think it's good enough. Trying to only display the necessary
parentheses might not worth the effort.
--
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]
Re: [I] Expr formatting missing parentheses [datafusion]
hendrikmakait commented on issue #16054:
URL: https://github.com/apache/datafusion/issues/16054#issuecomment-2920043776
I've had a closer look, we simply wrap `- {foo}` in parentheses regardless
of whether that's strictly necessary. Following the same principle for
`BinaryExpr` won't look as pretty as it could, but it would always be correct.
To avoid adding parentheses when not necessary, we'd have to pass more
information into `SchemaDisplay`. We'd need to be able to access the parent's
precedence to make a decision.
--
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]
Re: [I] Expr formatting missing parentheses [datafusion]
hendrikmakait commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-2919976201 It looks like we also misplace parentheses when combining binary expressions and unary expressions: ``` > select -(1+2); +-+ | (- Int64(1) + Int64(2)) | +-+ | -3 | +-+ ``` -- 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]
Re: [I] Expr formatting missing parentheses [datafusion]
hendrikmakait commented on issue #16054: URL: https://github.com/apache/datafusion/issues/16054#issuecomment-2910310089 @DanielFrantes: Are you still working on this? Otherwise, I'd love to take a stab at it. -- 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]
