GROUP BY with aggregate functions gives strange types

launchbadge / sqlx

🧰 The Rust SQL Toolkit. An async, pure Rust SQL crate featuring compile-time checked queries without a DSL. Supports PostgreSQL, MySQL, and SQLite.

Apache License 2.0

12.81k stars 1.22k forks source link

GROUP BY with aggregate functions gives strange types #3238

Open docwilco opened 3 months ago

docwilco commented 3 months ago

Bug Description

When using GROUP BY on a NON NULL column, said column becomes an Option<T> in combination with the MIN() and MAX() aggregate functions. Also MIN(), MAX(), SUM(), and COUNT() get i32 as type, not i64. As the testcase shows, AVG() and TOTAL() seem to behave correctly.

Minimal Reproduction

https://gist.github.com/docwilco/b5ea0972a21714a4d0f2c3e824b45c1e

Info

SQLx version: 0.7.4
SQLx features enabled: sqlite, runtime-tokio
Database server and version: SQLite <3.31.1>
Operating system: Ubuntu 20.04.4 (focal)
rustc --version: rustc 1.77.2 (25ef9e3d8 2024-04-09)

Also tried with:

Database server and version: SQLite <3.37.2>
Operating system: Ubuntu 22.04.4 (jammy) (upgraded the same machine from 20.04 to 22.04)
RustC both same version 1.77.2 and rustc 1.78.0 (9b00956e5 2024-04-29)

docwilco commented 3 months ago

0.7.4 outputs:

no GROUP BY: alloc::string::String/i64
GROUP BY only: alloc::string::String
GROUP BY + MIN(): core::option::Option<alloc::string::String>/i32
GROUP BY + MAX(): core::option::Option<alloc::string::String>/i32
GROUP BY + SUM(): alloc::string::String/i32
GROUP BY + COUNT(): alloc::string::String/i32
GROUP BY + AVG(): alloc::string::String/f64
GROUP BY + TOTAL(): alloc::string::String/f64

With main branch (0449ac5) I get:

no GROUP BY: alloc::string::String/i64
GROUP BY only: alloc::string::String
GROUP BY + MIN(): core::option::Option<alloc::string::String>/i64
GROUP BY + MAX(): core::option::Option<alloc::string::String>/i64
GROUP BY + SUM(): alloc::string::String/i64
GROUP BY + COUNT(): alloc::string::String/i64
GROUP BY + AVG(): alloc::string::String/f64
GROUP BY + TOTAL(): alloc::string::String/f64

So things are fixed except for the String becoming Option<String> for MIN() and MAX().

tyrelr commented 2 months ago

Thanks for the reproducer.

I tweaked your reproducer so that I could dump a trace of the potential opcode execution paths, then commented out the execution paths that were irrelevant, to see what is going on. https://gist.github.com/tyrelr/daf8e0ee29a2bcd978d1d95da199b5f7

At a glance I would guess that the issue is due to min/max not being implemented by the AggFinal command (as that seems to be where the null value comes from in the graph). But I haven't dug in, so that's just a guess.

The code to change would be in sqlx-sqlite/src/connection/explain.rs, if you want to give it a try.