Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: incorrect result for aliased arguments in select list after TreeNode rewrite #9678

Closed
Tracked by #9682
alamb opened this issue Mar 18, 2024 · 2 comments · Fixed by #9685
Closed
Tracked by #9682
Assignees
Labels
bug Something isn't working regression Something that used to work no longer does

Comments

@alamb
Copy link
Contributor

alamb commented Mar 18, 2024

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

  1. coalesce seems 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.
  1. It seems to have come in via Consolidate TreeNode transform 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.
  1. It happens for other functions (like cos) even though we only saw it for acos in 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 ABOVE

You can see coalesce(Float64(1) / m.f64, Float64(0)) AS acos(coalesce(Float64(1) / m.f64,Float64(0))) shows clearly the acos was removed

@alamb alamb added bug Something isn't working regression Something that used to work no longer does labels Mar 18, 2024
@alamb
Copy link
Contributor Author

alamb commented Mar 18, 2024

I believe @wiedld is looking into this

@wiedld
Copy link
Contributor

wiedld commented Mar 18, 2024

take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression Something that used to work no longer does
Projects
None yet
2 participants