fix: casting Int64 to Float64 unsuccessfully caused tpch8 to fail

[xudong963]

Which issue does this PR close?

Closes #1576 #165

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[xudong963]

TPCH 8

➜  benchmarks git:(fix_cast) ✗ cargo run --release --bin tpch -- benchmark datafusion --iterations 3 --path ./data --format tbl --query 8 --batch-size 4096
 
Running benchmarks with the following options: DataFusionBenchmarkOpt { query: 8, debug: false, iterations: 3, partitions: 2, batch_size: 4096, path: "./data", file_format: "tbl", mem_table: false }
Query 8 iteration 0 took 6401.8 ms
Query 8 iteration 1 took 5731.4 ms
Query 8 iteration 2 took 5830.1 ms
Query 8 avg time: 5987.78 ms

TPCH 14

➜  benchmarks git:(fix_cast) cargo run --release --bin tpch -- benchmark datafusion --iterations 3 --path ./data --format tbl --query 14 --batch-size 4096
 
Running benchmarks with the following options: DataFusionBenchmarkOpt { query: 14, debug: false, iterations: 3, partitions: 2, batch_size: 4096, path: "./data", file_format: "tbl", mem_table: false }
Query 14 iteration 0 took 4292.5 ms
Query 14 iteration 1 took 4464.5 ms
Query 14 iteration 2 took 2763.9 ms
Query 14 avg time: 3840.29 ms

[xudong963]

One problem with this modification is that float64 to int causes a loss of accuracy. I think return_type decided by then data type is unreasonable.

I checked Postgres' behavior, it can process reasonably.

postgres=# SELECT * FROM test;
 a
---
 1
 2
 3
(3 rows)


postgres=# SELECT a,
       CASE WHEN a=1 THEN 1.2
            ELSE 0
       END
    FROM test;
 a | case
---+------
 1 |  1.2
 2 |    0
 3 |    0
(3 rows)


postgres=# SELECT a,
       CASE WHEN a=1 THEN 12
            ELSE 5.4
       END
    FROM test;
 a | case
---+------
 1 |   12
 2 |  5.4
 3 |  5.4
(3 rows)

[houqp]

I think return_type decided by then data type is unreasonable.

I agree, better to take both branches into account.

[houqp]

Good job unlocking more TPCH queries!

[alamb]

Thanks @xudong963 !

[alamb]

Eventually it would be great to separate out the coercion logic into the `

/// Create a CASE expression
pub fn case(
    expr: Option<Arc<dyn PhysicalExpr>>,
    when_thens: &[WhenThen],
    else_expr: Option<Arc<dyn PhysicalExpr>>,
) -> Result<Arc<dyn PhysicalExpr>> {
    Ok(Arc::new(CaseExpr::try_new(expr, when_thens, else_expr)?))
}

function the way it is done with BinaryExpr and binary_cast

https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/physical_plan/expressions/binary.rs#L1106-L1114

But this is better than master so 👍

[xudong963]

I think return_type decided by then data type is unreasonable.

I agree, better to take both branches into account.

Next PR I'll do the thing and keep consistent with Postgres. cc @alamb

[alamb]

Filed #1609 to track