-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Display for Expr, improve operator display #971
Conversation
@alamb @Dandandan FYI i just started on this. I added the impl for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @matthewmturner !
I think this looks like a good approach. Thank you
datafusion/src/logical_plan/expr.rs
Outdated
ref right, | ||
ref op, | ||
} => write!(f, "{} {} {}", left, op, right), | ||
_ => write!(f, "{}", ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this code would cover all the variants of Expr
-- however, that doesn't have to be done in one PR.
How about as a potential interim solution we could do something like the following and use the debug display until someone has time to add a better display implementation?
_ => write!(f, "{}", ""), | |
_ => write!(f, "{:?}", self), |
@@ -117,19 +117,19 @@ mod tests { | |||
fn test_operators() { | |||
assert_eq!( | |||
format!("{:?}", lit(1u32) + lit(2u32)), | |||
"UInt32(1) Plus UInt32(2)" | |||
"UInt32(1) + UInt32(2)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sure looks better
Note the python test failure has been resolved on master so if you rebase this branch against apache/master that test should pass |
datafusion/src/sql/planner.rs
Outdated
@@ -2343,8 +2347,8 @@ mod tests { | |||
GROUP BY first_name | |||
HAVING MAX(age) > 100 AND MIN(id - 2) < 50"; | |||
let expected = "Projection: #person.first_name, #MAX(person.age)\ | |||
\n Filter: #MAX(person.age) Gt Int64(100) And #MIN(person.id Minus Int64(2)) Lt Int64(50)\ | |||
\n Aggregate: groupBy=[[#person.first_name]], aggr=[[MAX(#person.age), MIN(#person.id Minus Int64(2))]]\ | |||
\n Filter: #MAX(person.age) > Int64(100) AND #MIN(person.id Minus Int64(2)) < Int64(50)\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb Im a bit confused. The minus operator seems to be formatted differently depending on the part of the plan its in - is that expected? The test passes right now but i had to use Minus
in the Filter
section and -
in the Aggregate
section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One difference is that MIN
is a aggregate function which is a different type of Expr -- perhaps it formats its arguments using {:?}
efaba4c
to
ef99adf
Compare
@@ -64,7 +64,7 @@ pub enum AggregateFunction { | |||
impl fmt::Display for AggregateFunction { | |||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |||
// uppercase of the debug. | |||
write!(f, "{}", format!("{:?}", self).to_uppercase()) | |||
write!(f, "{}", self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb FYI after making this update I get a stack overflow error when running the test select_aggregate_with_group_by_with_having_using_derived_column_aggreagate_not_in_select
. I think i saw on some other issues/PRs a stack related issue, and im not sure if it could be related to this, but wanted to run it by you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#910 (comment) is a perhaps relevant comment. It adds RUST_MIN_STACK_SIZE
when the tests are run. Perhaps you can try rebasing to pick up the changes in #910 and see if the problem you were seeing is resolved?
ref left, | ||
ref right, | ||
ref op, | ||
} => write!(f, "{} {} {}", left, op, right), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used at the moment, as the implementation uses debug.
@@ -64,7 +64,7 @@ pub enum AggregateFunction { | |||
impl fmt::Display for AggregateFunction { | |||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |||
// uppercase of the debug. | |||
write!(f, "{}", format!("{:?}", self).to_uppercase()) | |||
write!(f, "{}", self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this doing infinite recursion, as it will try to use the same method to display self
?
Expr::BinaryExpr { left, op, right } => { | ||
write!(f, "{:?} {:?} {:?}", left, op, right) | ||
write!(f, "{:?} {} {:?}", left, op, right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dandandan FYI i updated here to use display and i think thats what made it work. i was able to get the expected results on some tests after this as well.
Are you ok with this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be ok with this as intermediate solution. Ideally we'll move the formatting to Display
instead and use that for printing @alamb wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Dandandan
Nice, some tests to go ;) |
@Dandandan @alamb I updated and got all tests to pass. let me know if anything else needed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great quality of life improvement 👍💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much nicer to me. Thank you @matthewmturner !
Expr::BinaryExpr { left, op, right } => { | ||
write!(f, "{:?} {:?} {:?}", left, op, right) | ||
write!(f, "{:?} {} {:?}", left, op, right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Dandandan
I am going to test this branch locally against main to make sure there are no logical conficts in the tests after merging #792 and if not will merge it in |
unfortunately when I merged apache/master into this branch locally, several tests failed. It looks like a few newly added tests also need to be updated
|
@matthewmturner let us know when you can have a look at the tests, I think only a rebase and update of the tests is what's needed here |
sry, somehow missed the prior comments. will work on this. |
as im relatively new to having to rebase, and ive had some issues in the past with it, i just want to confirm im doing it the right way. i was going to do can you confirm if this is ok or if there is a better way? thanks in advance! |
I normally do it by:
where I have previously set
But I suspect there are other workflows that work too |
@matthewmturner if you are not comfortable doing rebase, you can also do a simple |
Update logical_plan/operators tests rebase and debug display for non binary expr Add Display for Expr::BinaryExpr Update logical_plan/operators tests Updating tests Update aggregate display Updating tests without aggregate More tests Working on agg/scalar functions Fix binary_expr in create_name function and attendant tests More tests More tests Doc tests Rebase and update new tests
818b1d2
to
c856388
Compare
gah, guess i messed up the rebase. although im confused how the submodule was impacted - i didnt do anything for that (maybe that was the issue). ill look into it. |
@alamb all good now i think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @matthewmturner -- something is still messed up with the submodules as this PR shows a change to them.
Let me take a shot at cleaning up the PR and see if I can get rid of them...
@alamb ugh sry for the trouble. its not clear to me the right flow i should have performed. my understanding was that i was missing the updates to the submodules (avro in particular) so i updated them to the latest. what is the correct way to update submodules then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a fix in af86048. For the record, here are the commands I used (cargo culted from past experience):
git reset apache/master testing
git restore testing
git reset apache/master parquet-testing
git restore parquet-testing
git commit -a -m 'Restore submodule references from master'
git push
Thanks for sticking with this @matthewmturner -- I plan to merge this PR when the tests have completed
Ok - thank you for providing that. |
No worries! I personally find working with submodules with git also quite confusing -- part of the problem is that changes to them are picked up if you do |
Thanks again @matthewmturner ! |
@alamb np. appreciate your guidance and patience. |
Which issue does this PR close?
Closes #347
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?