Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve formatting of binary expressions #3884

Merged
merged 4 commits into from
Oct 25, 2022
Merged

Improve formatting of binary expressions #3884

merged 4 commits into from
Oct 25, 2022

Conversation

andygrove
Copy link
Member

Which issue does this PR close?

Closes #3865 and #3878

Rationale for this change

See issue for details

What changes are included in this PR?

Put parentheses around child binary expressions

Are there any user-facing changes?

@github-actions github-actions bot added core Core datafusion crate logical-expr Logical plan and expressions labels Oct 18, 2022
@github-actions github-actions bot added the optimizer Optimizer rules label Oct 18, 2022
@github-actions github-actions bot removed the optimizer Optimizer rules label Oct 22, 2022
@github-actions github-actions bot added the optimizer Optimizer rules label Oct 24, 2022
@andygrove andygrove marked this pull request as ready for review October 24, 2022 13:41
@github-actions github-actions bot added the sql label Oct 24, 2022
@andygrove
Copy link
Member Author

@Ted-Jiang could you help review?

@andygrove andygrove requested a review from alamb October 24, 2022 22:29
@Ted-Jiang
Copy link
Member

@Ted-Jiang could you help review?

Sure! I will do this today later.

Dandandan
Dandandan reviewed Oct 25, 2022
View reviewed changes
datafusion/expr/src/expr.rs
match expr {
Expr::BinaryExpr(child) => {
let p = child.precedence();
if p == 0 || p < precedence {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if p == 0 || p < precedence {
if p <= precedence {

Is the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point

Dandandan
Dandandan approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 looks really good

Ted-Jiang
Ted-Jiang approved these changes Oct 25, 2022
View reviewed changes
benchmarks/expected-plans/q19.txt
@@ -3,7 +3,7 @@ Projection: SUM(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount) AS re
Projection: lineitem.l_extendedprice, lineitem.l_discount
Filter: part.p_brand = Utf8("Brand#12") AND part.p_container IN ([Utf8("SM CASE"), Utf8("SM BOX"), Utf8("SM PACK"), Utf8("SM PKG")]) AND lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) AND part.p_size <= Int32(5) OR part.p_brand = Utf8("Brand#23") AND part.p_container IN ([Utf8("MED BAG"), Utf8("MED BOX"), Utf8("MED PKG"), Utf8("MED PACK")]) AND lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) AND part.p_size <= Int32(10) OR part.p_brand = Utf8("Brand#34") AND part.p_container IN ([Utf8("LG CASE"), Utf8("LG BOX"), Utf8("LG PACK"), Utf8("LG PKG")]) AND lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2) AND part.p_size <= Int32(15)
Inner Join: lineitem.l_partkey = part.p_partkey
Filter: lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) OR lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2) AND lineitem.l_shipmode IN ([Utf8("AIR"), Utf8("AIR REG")]) AND lineitem.l_shipinstruct = Utf8("DELIVER IN PERSON")
Filter: (lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) OR lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2)) AND lineitem.l_shipmode IN ([Utf8("AIR"), Utf8("AIR REG")]) AND lineitem.l_shipinstruct = Utf8("DELIVER IN PERSON")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 More clearly！

datafusion/optimizer/src/common_subexpr_eliminate.rs
@@ -671,8 +671,8 @@ mod test {
)?
.build()?;

let expected = "Aggregate: groupBy=[[]], aggr=[[SUM(test.a * Int32(1) - test.bInt32(1) - test.btest.bInt32(1)test.a AS test.a * Int32(1) - test.b), SUM(test.a * Int32(1) - test.bInt32(1) - test.btest.bInt32(1)test.a AS test.a * Int32(1) - test.b * Int32(1) + test.c)]]\
\n Projection: test.a * Int32(1) - test.b AS test.a * Int32(1) - test.bInt32(1) - test.btest.bInt32(1)test.a, test.a, test.b, test.c\
let expected = "Aggregate: groupBy=[[]], aggr=[[SUM(test.a * (Int32(1) - test.b)Int32(1) - test.btest.bInt32(1)test.a AS test.a * Int32(1) - test.b), SUM(test.a * (Int32(1) - test.b)Int32(1) - test.btest.bInt32(1)test.a AS test.a * Int32(1) - test.b * (Int32(1) + test.c))]]\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

(Int32(1) - test.b)Int32(1)

this is a little confused, should we change it to 🤔

(Int32(1) - test.bInt32(1))

Maybe we can file another issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is related to #3786 ?

@Dandandan Dandandan merged commit 7559c44 into apache:master Oct 25, 2022
@andygrove andygrove deleted the binary-expr-fmt branch October 25, 2022 14:05
@andygrove andygrove mentioned this pull request Oct 25, 2022
Open
alamb
alamb reviewed Oct 25, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @andygrove

@ursabot
Copy link

ursabot commented Oct 25, 2022

Benchmark runs are scheduled for baseline = 7cba758 and contender = 7559c44. 7559c44 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Dandandan pushed a commit to yuuch/arrow-datafusion that referenced this pull request Nov 5, 2022
@andygrove @Dandandan
Improve formatting of binary expressions (apache#3884)
7c4cdc3 
* Respect operator precedence when writing binary expressions

* update tests

* update tests

* refactor to reduce duplicate code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb left review comments

@HaoYang670 HaoYang670 HaoYang670 left review comments

@Dandandan Dandandan Dandandan approved these changes

@Ted-Jiang Ted-Jiang Ted-Jiang approved these changes

Assignees
No one assigned
Labels
core Core datafusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Binary expression canonical names are incorrect in some cases
6 participants
@andygrove @Ted-Jiang @ursabot @Dandandan @alamb @HaoYang670