Skip to content

Conversation

@jonahgao
Copy link
Member

@jonahgao jonahgao commented Aug 25, 2023

Which issue does this PR close?

Closes #3849
Closes #4669

Now both 0 / 0 and 10 / 0 will result in the same DivideByZero error.

DataFusion CLI v30.0.0
❯ select 0/0;
Optimizer rule 'simplify_expressions' failed
caused by
Arrow error: Divide by zero error
❯ select 10/0;
Optimizer rule 'simplify_expressions' failed
caused by
Arrow error: Divide by zero error

Rationale for this change

The const evaluator will first eliminate the expression 0 / 0 and raise a DivideByZero error.
https://github.com/apache/arrow-datafusion/blob/ea9144e6597593c09a4ef0b71a4da1cdfaca8249/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L129-L130
Then this rule will never have a match.
https://github.com/apache/arrow-datafusion/blob/ea9144e6597593c09a4ef0b71a4da1cdfaca8249/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L709-L715

Furthermore, the behavior of this rule is inconsistent with the results of the following test case.
https://github.com/apache/arrow-datafusion/blob/ea9144e6597593c09a4ef0b71a4da1cdfaca8249/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L1666-L1675

What changes are included in this PR?

Remove an unreached simplification rule.

Are these changes tested?

N/A

Are there any user-facing changes?

No

@github-actions github-actions bot added the optimizer Optimizer rules label Aug 25, 2023
@alamb
Copy link
Contributor

alamb commented Aug 25, 2023

I believe CI will pass after merging up from main to get the change in #7416

1 similar comment
@alamb
Copy link
Contributor

alamb commented Aug 25, 2023

I believe CI will pass after merging up from main to get the change in #7416

@alamb
Copy link
Contributor

alamb commented Aug 25, 2023

I took the liberty of merging up from main to get a fix for CI failure

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.

This change makes sense to me -- thank you @jonahgao

Copy link
Member

@Weijun-H Weijun-H 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 alamb merged commit 193cdcf into apache:main Aug 26, 2023
@alamb
Copy link
Contributor

alamb commented Aug 26, 2023

Thanks @jonahgao and @Weijun-H

@jonahgao jonahgao deleted the divide_zero_rule branch August 26, 2023 12:28
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.

inconsistent behavior for divide zero: divide_opt and divide_scalar Inconsistent Error Schema in DataFusion

3 participants