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

Refactor: Consolidate expression simplification code in simplify_expression.rs #1374

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Nov 28, 2021

NOTE: This PR just moves code around (it doesn't change logic) so please don't be too scared of its size

Which issue does this PR close?

Re #1160 (more sophisticated expression simplification)

Rationale for this change

There are at least three places where expressions are simplified, with some overlap: const_evaluator.rs, expression_simplifier.rs, and optimizer/util.rs

This makes it hard both to understand what simplifications are applied, as well as what is redundant (e.g. #1164)

What changes are included in this PR?

  1. Move ConstEvaluator and Simplifier logic and tests into expression_simplifier.rs.
  2. Rename some tests

Are there any user-facing changes?

The location of ConstEvaluator has changed

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Nov 28, 2021
@alamb
Copy link
Contributor Author

alamb commented Nov 28, 2021

FYI @rdettai this is one strategy to hopefully make reviews a bit easier: the first PR is mechanical and hopefully quick to review. The second PR can then have logic changes in it without as many mechanical changes.

@alamb
Copy link
Contributor Author

alamb commented Nov 28, 2021

FYI @houqp / @jgoday / @liukun4515 -- I plan a bit of consolidation in the expression simplification logic. See this PR and the ones linked to it

@liukun4515
Copy link
Contributor

FYI @houqp / @jgoday / @liukun4515 -- I plan a bit of consolidation in the expression simplification logic. See this PR and the ones linked to it

I will review this later.
But the context is too much, I need more time to understand them.
Maybe takes two or three days.

@alamb

@alamb
Copy link
Contributor Author

alamb commented Nov 29, 2021

I will review this later.

Thank you @liukun4515 -- this PR in particular looks massive but in reality is pretty simple -- I literally cut/pasted code from one module to another

@alamb alamb changed the title Consolidate expression simplification code in simplify_expression.rs Refactor: Consolidate expression simplification code in simplify_expression.rs Nov 29, 2021
@alamb
Copy link
Contributor Author

alamb commented Nov 29, 2021

I don't actually think this change requires much review (as it just moves stuff around) so unless anyone has any objections, I'll plan to merge it sometime tomorrow

@houqp houqp added the api change Changes the API exposed to users of the crate label Nov 29, 2021
Copy link
Contributor

@liukun4515 liukun4515 left a comment

Choose a reason for hiding this comment

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

LGTM

@alamb
Copy link
Contributor Author

alamb commented Nov 30, 2021

Thank you @houqp and @liukun4515

@alamb alamb merged commit 5b23180 into apache:master Nov 30, 2021
@alamb alamb deleted the alamb/consolidate_constant_folding branch November 30, 2021 11:44
@alamb alamb removed the api change Changes the API exposed to users of the crate label Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants