-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Closed
Labels
bugSomething isn't workingSomething isn't workingregressionSomething that used to work no longer doesSomething that used to work no longer does
Description
Describe the bug
While updating DataFusion in InfluxDB we found what appears to be a bug in common_subexpr_eliminate that causes incorrect answers that seems to have come in via the TreeNode refactor in #8891
To Reproduce
> create table m(f64 double) as values (10.1);
0 rows in set. Query took 0.038 seconds.
-- This query is correct (note that acos is 1.471623942988976)
❯ select f64, coalesce(1.0 / f64, 0.0), acos(coalesce(1.0 / f64, 0.0)) from m;
+------+-----------------------------------------+-----------------------------------------------+
| f64 | coalesce(Float64(1) / m.f64,Float64(0)) | acos(coalesce(Float64(1) / m.f64,Float64(0))) |
+------+-----------------------------------------+-----------------------------------------------+
| 10.1 | 0.09900990099009901 | 1.471623942988976 |
+------+-----------------------------------------+-----------------------------------------------+
1 row in set. Query took 0.001 seconds.
-- However, when I add an alias `as f64_1` to the second argument the query is now incorrect (the acos ...) term now is `0.09900990099009901 which is the same as 1/10.1`)
❯ select f64, coalesce(1.0 / f64, 0.0) as f64_1, acos(coalesce(1.0 / f64, 0.0)) from m;
+------+---------------------+-----------------------------------------------+
| f64 | f64_1 | acos(coalesce(Float64(1) / m.f64,Float64(0))) |
+------+---------------------+-----------------------------------------------+
| 10.1 | 0.09900990099009901 | 0.09900990099009901 |
+------+---------------------+-----------------------------------------------+
1 row in set. Query took 0.002 seconds.
Expected behavior
The correct answer is:
❯ select f64, coalesce(1.0 / f64, 0.0) as f64_1, acos(coalesce(1.0 / f64, 0.0)) from m;
+------+---------------------+-----------------------------------------------+
| f64 | f64_1 | acos(coalesce(Float64(1) / m.f64,Float64(0))) |
+------+---------------------+-----------------------------------------------+
| 10.1 | 0.09900990099009901 | 1.471623942988976 |
+------+---------------------+-----------------------------------------------+
1 row in set. Query took 0.008 seconds.Additional context
coalesceseems to be required to trigger the bug. Removing it from the query results in
❯ select f64, 1.0/f64 as f64_1, acos(1.0 / f64) from m;
+------+---------------------+--------------------------+
| f64 | f64_1 | acos(Float64(1) / m.f64) |
+------+---------------------+--------------------------+
| 10.1 | 0.09900990099009901 | 1.471623942988976 |
+------+---------------------+--------------------------+
1 row in set. Query took 0.003 seconds.- It seems to have come in via Consolidate
TreeNodetransform and rewrite APIs #8891
The bug happens in a84e5f8, but does not happen in 684b4fa (the commit right before #8891)
❯ select f64, coalesce(1.0 / f64, 0.0) as f64_1, acos(coalesce(1.0 / f64, 0.0)) from m;
+------+---------------------+-----------------------------------------------+
| f64 | f64_1 | acos(coalesce(Float64(1) / m.f64,Float64(0))) |
+------+---------------------+-----------------------------------------------+
| 10.1 | 0.09900990099009901 | 1.471623942988976 |
+------+---------------------+-----------------------------------------------+
1 row in set. Query took 0.013 seconds.- It happens for other functions (like
cos) even though we only saw it foracosin IOx
I tracked the issue down to a problem in common_subexpr_eliminate. You can see the issue via explain verbose:
❯ explain verbose select f64, coalesce(1.0 / f64, 0.0) as f64_1, acos(coalesce(1.0 / f64, 0.0)) from m;
+------------------------------------------------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type | plan |
+------------------------------------------------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| initial_logical_plan | Projection: m.f64, coalesce(Float64(1) / m.f64, Float64(0)) AS f64_1, acos(coalesce(Float64(1) / m.f64, Float64(0))) |
| | TableScan: m |
...
|
| logical_plan after eliminate_cross_join | SAME TEXT AS ABOVE |
| logical_plan after common_sub_expression_eliminate | Projection: m.f64, coalesce(Float64(1) / m.f64, Float64(0)) AS f64_1, coalesce(Float64(1) / m.f64, Float64(0)) AS acos(coalesce(Float64(1) / m.f64,Float64(0))) |
| | Projection: coalesce(Float64(1) / m.f64, Float64(0)) AS coalesce(Float64(1) / m.f64, Float64(0)), m.f64 |
| | TableScan: m |
| logical_plan after eliminate_limit | SAME TEXT AS ABOVE |
| logical_plan after propagate_empty_relation | SAME TEXT AS ABOVE |
| logical_plan after eliminate_one_union | SAME TEXT AS ABOVEYou can see coalesce(Float64(1) / m.f64, Float64(0)) AS acos(coalesce(Float64(1) / m.f64,Float64(0))) shows clearly the acos was removed
Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't workingregressionSomething that used to work no longer doesSomething that used to work no longer does