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.

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

Improve expression simplification by running it twice #3811

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 12, 2022

Draft as it builds on #3741

Which issue does this PR close?

re #3740
re #1160

Rationale for this change

There are some fairly simple expressions that should be simplified but yet are not because additional simplifications are unlocked by the constant propagator.

For example

 (i * 0) > 5

is only simplified to

0 > 5

Because the flow is

  • constant evaluator runs on (i * 0) > 5 which can not do anything,
  • simplfy runs on (i * 0) > 5 and rewrites to 0 > 5

Now, if the constant evaluator can run again it will result in false which is great. However, it is not run again and thus there is the issue.

I hit the same basic embarrassment while making the example in re #1160

The challenges with a fully general solution to this issue are described #1160. However, we have had good luck in IOx simply running the simplification / constant prop twice

What changes are included in this PR?

  1. run evaluator / constant prop twice
  2. Add some more tests

Are there any user-facing changes?

Better constant propagation

@github-actions github-actions bot added core Core DataFusion crate optimizer Optimizer rules labels Oct 12, 2022
@alamb alamb marked this pull request as ready for review October 12, 2022 17:30
@alamb alamb changed the title Improve simplification by running it twice Improve expression simplification by running it twice Oct 12, 2022
@github-actions github-actions bot removed the core Core DataFusion crate label Oct 12, 2022
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM

@andygrove andygrove merged commit 37fe938 into apache:master Oct 12, 2022
@ursabot
Copy link

ursabot commented Oct 12, 2022

Benchmark runs are scheduled for baseline = 7179c54 and contender = 37fe938. 37fe938 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

@alamb alamb deleted the alamb/simplify_twice branch August 8, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants