Re: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
github-actions[bot] closed pull request #16209: Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` URL: https://github.com/apache/datafusion/pull/16209 -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
github-actions[bot] commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-3208719981 Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
alamb commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2978143366 Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
hendrikmakait commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2955545609 Sounds good, I'll take care of that when I have some more time next week. -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
alamb commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2953942139 > @alamb: That's a fair point, I actually brought it up in the issue ([#16054 (comment)](https://github.com/apache/datafusion/issues/16054#issuecomment-2924301139)). I'm happy to adjust the implementation. I think we should try to adjust if possible. Thank you @hendrikmakait -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
hendrikmakait commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2948532708 @alamb: That's a fair point, I actually brought it up in the issue (https://github.com/apache/datafusion/issues/16054#issuecomment-2924301139). I'm happy to adjust the implementation. -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
xudong963 commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2940212614 > @xudong963: CI is green now. cc @alamb for another look -- 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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
alamb commented on code in PR #16209:
URL: https://github.com/apache/datafusion/pull/16209#discussion_r2126757732
##
datafusion-examples/examples/date_time_functions.rs:
##
@@ -122,15 +122,15 @@ async fn query_make_date() -> Result<()> {
let df = ctx.sql("select make_date(y + 1, m, d) from t").await?;
let expected = [
-"+---+",
-"| make_date(t.y + Int64(1),t.m,t.d) |",
-"+---+",
-"| 2021-01-15|",
-"| 2022-02-16|",
-"| 2023-03-17|",
-"| 2024-04-18|",
-"| 2025-05-19|",
-"+---+",
+"+-+",
+"| make_date((t.y + Int64(1)),t.m,t.d) |",
Review Comment:
The parens here seem unecessary (the top most expression) -- I wonder if we
can avoid that somehow
--
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: [PR] Always add parentheses when formatting `BinaryExpr` with `SchemaDisplay` [datafusion]
hendrikmakait commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2939950601 @xudong963: CI is green now. -- 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: [PR] Always add parentheses when formatting`BinaryExpr` with `SchemaDisplay` [datafusion]
xudong963 commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2934084892 @hendrikmakait Hi, there are still a few places that need to change -- 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: [PR] Always add parentheses when formatting`BinaryExpr` with `SchemaDisplay` [datafusion]
hendrikmakait commented on code in PR #16209:
URL: https://github.com/apache/datafusion/pull/16209#discussion_r2118214706
##
datafusion/expr/src/expr.rs:
##
@@ -2500,7 +2500,7 @@ impl Display for SchemaDisplay<'_> {
}
}
Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
-write!(f, "{} {op} {}", SchemaDisplay(left),
SchemaDisplay(right),)
+write!(f, "({} {op} {})", SchemaDisplay(left),
SchemaDisplay(right),)
Review Comment:
This is the actual change.
--
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: [PR] Always add parentheses when formatting`BinaryExpr` with `SchemaDisplay` [datafusion]
alamb commented on PR #16209: URL: https://github.com/apache/datafusion/pull/16209#issuecomment-2921965832 Given how widely used this code is I suspect this change will be quite invasive / require changing many tests -- 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]
