Skip to content

Conversation

@izveigor
Copy link
Contributor

@izveigor izveigor commented Mar 8, 2023

Which issue does this PR close?

Follow on to #5423

Rationale for this change

I propose to add the symmetric optimization function for bitwise negative.
Rules for bitwise negative in case of a binary expression are the same as the not clause:
~(A & B) ===> ~A | ~B
~(A | B) ===> ~A & ~B
~(~A) ===> A

What changes are included in this PR?

New optimization method for binary expressions with bitwise negative.

Are these changes tested?

After PR merge: #5511

Are there any user-facing changes?

Should increase execution speed

@github-actions github-actions bot added the optimizer Optimizer rules label Mar 8, 2023
/// ~(A | B) ===> ~A & ~B
/// ~(~A) ===> A
/// For others, use Negative clause
pub fn negative_clause(expr: Expr) -> Expr {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, it deserves a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is basically applying https://en.wikipedia.org/wiki/De_Morgan%27s_laws

Maybe we can call it distribute_negation?

@izveigor
Copy link
Contributor Author

izveigor commented Mar 8, 2023

Regarding tests: I have not noticed relevant tests for this segment of the code: https://github.com/apache/arrow-datafusion/blob/main/datafusion/optimizer/src/simplify_expressions/utils.rs#L249-#L273. Should we create a separate PR for this case to keep the symmetry?

@alamb
Copy link
Contributor

alamb commented Mar 9, 2023

Should we create a separate PR for this case to keep the symmetry?

Yes please. Or even better I wonder if there is a way to avoid the duplication.

Thank you @izveigor

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.

Thanks @izveigor -- I think this PR needs some tests as you point out. I will try and prioritize getting #5511 in shortly

/// ~(A | B) ===> ~A & ~B
/// ~(~A) ===> A
/// For others, use Negative clause
pub fn negative_clause(expr: Expr) -> Expr {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is basically applying https://en.wikipedia.org/wiki/De_Morgan%27s_laws

Maybe we can call it distribute_negation?

@alamb alamb marked this pull request as draft March 9, 2023 18:53
@alamb
Copy link
Contributor

alamb commented Mar 9, 2023

Converted to draft as it is waiting on other PRs and tests

@izveigor izveigor marked this pull request as ready for review March 12, 2023 08:55
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.

Looks good -- thank you @izveigor

@alamb alamb merged commit 3496769 into apache:main Mar 15, 2023
@andygrove andygrove added the enhancement New feature or request label Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants