AndreaBozzo commented on PR #19916: URL: https://github.com/apache/datafusion/pull/19916#issuecomment-3824013080
> Why does this PR remove the logic to wrap children based on operator precedence in `Display`? It feels like an odd choice, with the reasoning provided as: > > > This simplifies the previous precedence-based logic and avoids ambiguity > > The previous logic was already quite simple; and were there cases where it was leading to ambiguity? > > If anything I feel `SqlDisplay` logic should follow suit in using precedence to decide when to use parentheses, to avoid needing to wrap every binary operator with them 🤔 Yes, the old `Display` correctly handled that, i got lost trying to find a "smart workaround" after realizing i couldn't change the CLI headers ( which the original issue points to), my approach is way too forced and clunky. I can potentially revert `Display` and apply precedence based logic on `SqlDisplay` if you think there's some value left on this PR. Thanks again for your review -- 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]
