Skip to content

fix: misc phys. expression display bugs - #5387

Merged
avantgardnerio merged 3 commits into
apache:mainfrom
crepererum:crepererum/issue4695b
Feb 24, 2023
Merged

fix: misc phys. expression display bugs#5387
avantgardnerio merged 3 commits into
apache:mainfrom
crepererum:crepererum/issue4695b

Conversation

@crepererum

@crepererum crepererum commented Feb 24, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Found while working on #4695.

Rationale for this change

Broken display code makes debugging and testing hard.

What changes are included in this PR?

  • TryCast should display as TRY_CAST, not as CAST
  • BinaryExpr should use parenthesis

Are these changes tested?

  • fix the two display bugs mentioned above
  • move precedence from logical binary expression to Operator, so I can reuse this in the phys. expr. display code

Are there any user-facing changes?

Slight changes in debug outputs.

Breaking: BinaryExpr::precedence is gone

@github-actions github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Feb 24, 2023
waynexia
waynexia approved these changes Feb 24, 2023
View reviewed changes

@waynexia waynexia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The new format looks pretty 🤩

Comment thread datafusion/physical-expr/src/expressions/binary.rs
lit(ScalarValue::from(4)),
)),
);
assert_eq!(expr.to_string(), "(1 OR 2) AND (3 OR 4)");

@waynexia waynexia Feb 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@avantgardnerio avantgardnerio Feb 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new format looks pretty star_struck

Agreed

@avantgardnerio
avantgardnerio merged commit cef119d into apache:main Feb 24, 2023
@ursabot

ursabot commented Feb 24, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ac33ebc and contender = cef119d. cef119d 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

@andygrove andygrove added the bug Something isn't working label Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avantgardnerio avantgardnerio avantgardnerio approved these changes
@waynexia waynexia waynexia approved these changes

Assignees

No one assigned

Labels

bug Something isn't working logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@crepererum @ursabot @avantgardnerio @waynexia @andygrove