col / col returns 1 when col = 0

[HaoYang670]

Describe the bug
A clear and concise description of what the bug is.
The expression col / col always returns 1 even if col = 0

To Reproduce
Steps to reproduce the behavior:

❯ create table t as select 0 as a;
0 rows in set. Query took 0.002 seconds.
❯ select a / a from t;
+-----------+
| t.a / t.a |
+-----------+
| 1         |
+-----------+
1 row in set. Query took 0.003 seconds.
❯ select 0 / 0 from t;
+---------------------+
| Int64(0) / Int64(0) |
+---------------------+
|                     |
+---------------------+

Expected behavior
A clear and concise description of what you expected to happen.

col / col should return null when col = 0

Additional context
Add any other context about the problem here.

[andygrove]

Nice find!

[HaoYang670]

Should we remove the simplification A / A -> 1? https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/simplify_expressions.rs#L758-L763. I'm not sure if this would cause performance regression

[retikulum]

Hi. While I was looking for good first issues, I found this one. I am not a professional developer (try to improve my rust skills) and it is my first try to contribute a project.

I would like to ask if i am doing things right. Sorry, if I waste your time.

Generally, how do you handle null? I used Expr::Literal(ScalarValue::Int32(None)) for this implementation.

First I added 0 / 0 condition as BinaryExp just like other operators. You can find the code below:

            // 0 / 0 -> null
            BinaryExpr {
                left,
                op: Divide,
                right,
            } if is_zero(&left) && is_zero(&right) => Expr::Literal(ScalarValue::Int32(None)),

Secondly, I changed https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/simplify_expressions.rs#L798 to this:

} if !info.nullable(&left)? && !info.nullable(&right)? && (left == right) && !( is_zero(&left) && is_zero(&right) ) => lit(1),

I wrote test but I am not sure about quality.

    #[test]
    fn test_simplify_divide_zero_by_zero() {
        let expr = binary_expr(lit(0), Operator::Divide, lit(0));
        let expected = Expr::Literal(ScalarValue::Int32(None));

        assert_eq!(simplify(expr), expected);
    }

Thanks in advance :)

[HaoYang670]

Thank you, @retikulum, for taking this.
There are 2 issues here.

1. If the divisor is a literal zero, we should return the DivideByZeroError directly, just like what we have done for the modulo operator: https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/simplify_expressions.rs#L822-L829
2. For the A / A case, my suggestion is to remove the rule A / A -> lit(1), because we can't know whether A contains zeros when A is not a literal. An example in the postgresql is here: https://onecompiler.com/postgresql/3yjp52zuz

[retikulum]

Hi @HaoYang670, Thanks for your reply. You can find my comments about issues.

1. I just wrote this piece of code with its test:

            // A / 0 --> DivideByZeroError 
            BinaryExpr {
                left: _,
                op: Divide,
                right,
            } if is_zero(&right) => {
                return Err(DataFusionError::ArrowError(ArrowError::DivideByZero))
            }

    #[test]
    #[should_panic(
        expected = "called `Result::unwrap()` on an `Err` value: ArrowError(DivideByZero)"
    )]
    fn test_simplify_divide_by_zero() {
        let expr = binary_expr(col("c2_non_null"), Operator::Divide, lit(0));

        simplify(expr);
    }

2. I am not sure if I understood correctly but can't we check it with the following control !( is_zero(&left) && is_zero(&right) ) ? You mentioned about performance concerns above ( col / col returns 1 when col = 0 #3615 (comment)) so I am not sure what to do about it.

[HaoYang670]

Thank you, @retikulum. Your test case makes sense to me. 👍

can't we check it with the following control !( is_zero(&left) && is_zero(&right) )?

Sorry, we couldn't. Because is_zero only works for literals: https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/simplify_expressions.rs#L105.
If A is col(a), is_zero(A) will always return false. In another word, the value of col(a) is unknown.

You mentioned about performance concerns.
Yes, there might be some performance regression, but not much. Because this is just an expression simplification for a special case. Besides, correctness should be always over the performance.

[retikulum]

Thank you for your feedback. I will create a PR, if 0/0 and A/0 rules are correct and removing A/A is the last decision.