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

sql result discrepency with sqlite, postgres and duckdb bug #2 #13782

Closed
Tracked by #13811
Omega359 opened this issue Dec 14, 2024 · 6 comments
Closed
Tracked by #13811

sql result discrepency with sqlite, postgres and duckdb bug #2 #13782

Omega359 opened this issue Dec 14, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@Omega359
Copy link
Contributor

Describe the bug

External error: query result mismatch:
[SQL] SELECT - ( 74 ) * 62 + - 75 * + - 12 * + 68 * - + COUNT ( DISTINCT + 7 ) * 52 * + + COALESCE ( - + SUM ( ALL - 67 ), - 94 - - - NULLIF ( - 75, CAST ( NULL AS REAL ) * - CASE WHEN NULL IS NULL THEN 88 END + - COUNT ( * ) ), ( CASE + 18 WHEN 37 THEN NULL WHEN + 98 THEN NULL WHEN 13 THEN NULL ELSE 13 * 45 END ) * - 19 ) col1
[Diff] (-expected|+actual)
-   -213225388
+   -213225390
at test_files/sqlite/random/expr/slt_good_86.slt:34180

Datafusion:

> SELECT - ( 74 ) * 62 + - 75 * + - 12 * + 68 * - + COUNT ( DISTINCT + 7 ) * 52 * + + COALESCE ( - + SUM ( ALL - 67 ), - 94 - - - NULLIF ( - 75, CAST ( NULL AS REAL ) * - CASE WHEN NULL IS NULL THEN 88 END + - COUNT ( * ) ), ( CASE + 18 WHEN 37 THEN NULL WHEN + 98 THEN NULL WHEN 13 THEN NULL ELSE 13 * 45 END ) * - 19 ) col1;
+--------------+
| col1         |
+--------------+
| -213225390.0 |
+--------------+

duckdb and postgres:

D SELECT - ( 74 ) * 62 + - 75 * + - 12 * + 68 * - + COUNT ( DISTINCT + 7 ) * 52 * + + COALESCE ( - + SUM ( ALL - 67 ), - 94 - - - NULLIF ( - 75, CAST ( NULL AS REAL ) * - CASE WHEN NULL IS NULL THEN 88 END + - COUNT ( * ) ), ( CASE + 18 WHEN 37 THEN NULL WHEN + 98 THEN NULL WHEN 13 THEN NULL ELSE 13 * 45 END ) * - 19 ) col1;
┌────────────┐
│    col1    │
│   int128   │
├────────────┤
│ -213225388 │
└────────────┘
D

Duckdb and sqlite have the result as an int, DF and postgres as float.

To Reproduce

sql above.

Expected behavior

No response

Additional context

No response

@Omega359 Omega359 added the bug Something isn't working label Dec 14, 2024
@Omega359
Copy link
Contributor Author

Another example of what is probably the same cause:

Datafusion

>  SELECT ALL + + 84 * + - COUNT ( * ) * - - COALESCE ( + 37, + 14 ) * - NULLIF ( CASE + + COUNT ( * ) WHEN 54 THEN NULL ELSE + 91 + + - 60 * - 23 END, ( + CAST ( + 96 AS REAL ) ) ) * + 14 col2;
+------------+
| col2       |
+------------+
| 64006150.0 |
+------------+
1 row(s) fetched. 

duckdb and postgres:

D SELECT ALL + + 84 * + - COUNT ( * ) * - - COALESCE ( + 37, + 14 ) * - NULLIF ( CASE + + COUNT ( * ) WHEN 54 THEN NULL ELSE + 91 + + - 60 * - 23 END, ( + CAST ( + 96 AS REAL ) ) ) * + 14 col2;;
┌──────────┐
│   col2   │
│  int64   │
├──────────┤
│ 64006152 │
└──────────┘

@aweltsch
Copy link
Contributor

aweltsch commented Jan 3, 2025

Out of interest I looked into this a bit and wanted to share my findings.
To me it looks like this might be related to the type coercion of NULLIF.
I reduced the example to the following query:
datafusion cli:

> SELECT - 4588 - 213220800 * NULLIF (1, CAST ( NULL AS REAL )) col1;
+--------------+
| col1         |
+--------------+
| -213225390.0 |
+--------------+

duckdb

D SELECT - 4588 - 213220800 * NULLIF (1, CAST ( NULL AS REAL )) col1;
┌────────────┐
│    col1    │
│   int32    │
├────────────┤
│ -213225388 │
└────────────┘

Looking at just the nullif:
datafusion-cli:

SELECT NULLIF (1, CAST ( NULL AS REAL ));
+-----------------------+
| nullif(Int64(1),NULL) |
+-----------------------+
| 1.0                   |
+-----------------------+

duckdb

SELECT NULLIF (1, CAST ( NULL AS REAL ));
┌──────────────────────────────────┐
│ "nullif"(1, CAST(NULL AS FLOAT)) │
│              int32               │
├──────────────────────────────────┤
│                                1 │
└──────────────────────────────────┘

The documentation of nullif mentions that this is slightly different than postgres / duckdb:

// Documentation mentioned in Postgres,
// The result has the same type as the first argument — but there is a subtlety.
// What is actually returned is the first argument of the implied = operator,
// and in some cases that will have been promoted to match the second argument's type.
// For example, NULLIF(1, 2.2) yields numeric, because there is no integer = numeric operator, only numeric = numeric
//
// We don't strictly follow Postgres or DuckDB for **simplicity**.
// In this function, we will coerce arguments to the same data type for comparison need. Unlike DuckDB
// we don't return the **original** first argument type but return the final coerced type.
//
// In Postgres, nullif('2', 2) returns Null but nullif('2::varchar', 2) returns error.
// While in DuckDB both query returns Null. We follow DuckDB in this case since I think they are equivalent thing and should
// have the same result as well.

Maybe this helps someone with the investigation.

@Omega359
Copy link
Contributor Author

Omega359 commented Jan 3, 2025

It's not the nullif, it's the real datatype:

> SELECT 1 - 213220800 * 1::REAL;
+----------------------------------------+
| Int64(1) - Int64(213220800) * Int64(1) |
+----------------------------------------+
| -213220800.0                           |
+----------------------------------------+
1 row(s) fetched. 
Elapsed 0.001 seconds.

> SELECT 1 - 213220800 * 1.0;
+------------------------------------------+
| Int64(1) - Int64(213220800) * Float64(1) |
+------------------------------------------+
| -213220799.0                             |
+------------------------------------------+
1 row(s) fetched. 
Elapsed 0.001 seconds.

> SELECT 1 - 213220800 * 1;
+----------------------------------------+
| Int64(1) - Int64(213220800) * Int64(1) |
+----------------------------------------+
| -213220799                             |
+----------------------------------------+
1 row(s) fetched. 
Elapsed 0.001 seconds.

There is something with the 'real' datatype that is breaking things. I am unsure why 1::REAL is showing as Int64(1) in the first select statement, I am pretty sure it should be a float.

@aweltsch
Copy link
Contributor

aweltsch commented Jan 4, 2025

Thanks for your response @Omega359, I agree that the unexpected result is calculated due to calculations using the REAL type.
For me the main surprise was that nullif(1, 2::REAL) is of REAL type in datafusion,
since I don't think this is the case with some other DB systems like duckdb (integer) or postgres (double precision).

I also agree that SELECT 1::REAL is shown as Int64(1) looks like a bug, but I'm not sure
if that is related to REAL only, because

SELECT 1::DOUBLE;
+----------+
| Int64(1) |
+----------+
| 1.0      |
+----------+

is also shown as Int64 in datafusion. Using arrow_typeof instead shows what I would expect

> SELECT arrow_typeof(1::DOUBLE);
+------------------------+
| arrow_typeof(Int64(1)) |
+------------------------+
| Float64                |
+------------------------+

> SELECT arrow_typeof(1::REAL);
+------------------------+
| arrow_typeof(Int64(1)) |
+------------------------+
| Float32                |
+------------------------+

@Omega359
Copy link
Contributor Author

After fixing the test suite locally to use float8 instead of real (to mirror the actual data type that sqlite uses - 8 bytes for real vs 4 bytes for DF/postgres) the errors in this issue no longer occur.

@Omega359
Copy link
Contributor Author

Resolved as fixed with change 'as REAL' to 'AS FLOAT8' in sqlite test files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants