GeezerVison

Can we add this test case as well please?

-- name: GetNullable3B :many
SELECT CASE WHEN true THEN 'hello'::text ELSE null::text END 
FROM "mytable";

Where in your tests are we setting the expectation of the generated results?
Like, I would have expected your GetNullable2 to return an any/interface{}, but I don't see the expectation set anywhere. Right now, the generated code appears to return a pgtype.Text, which is odd for something returning integers, floats, and strings.

Or is the committed code the expectation?

Can we add this test case as well please?

GetNullable3B added

Like, I would have expected your GetNullable2 to return an any/interface{}

In my PR description I proposed type precedence, for multiple type inside case expression. For example

• 1st condition result is int, 2nd condition result is float, 3rd condition result is text, and default condition is null, then it should be nullable text, not an interface
• 1st condition result is int, 2nd condition result is float, and default condition is int, then it should be non-nullable float, not an interface
• 1st condition result is int, and default condition is null, then it should be nullable int, not an interface

and so on. but on the other side, I think that the user should be given freedom if the return value of case expr has several type variance (in short, just return any or interface{}). @kyleconroy maybe you have feedback on this particular case? 👀

Or is the committed code the expectation?

I have generated the result using code from this PR, and then make it as expectation (commited on repo), but to be honest I have a problem in github workflow tests, it seems that the binary that the workflow use is the previous version before I made these changes, so the generated go file would always be different than the one I commited on the repo. Especially on TestReplay endtoend tests. Maybe somebody can help giving me some pointers?

So after thinking about it, I realized we really only care about what Postgres thinks the returned values are, not what we think they should be.
I tried various combinations of your GetNullable2, and found that postgres doesn't really care about the order of which case/when statement comes first, but instead has some kind of preference, which you mentioned.

For example, if you insert at least 4 rows into mytable, then running this query:

SELECT CASE
    WHEN id = 1 THEN '1'
    WHEN id = 2 THEN null
    WHEN id = 3 THEN id
    WHEN id = 4 THEN 4.5
    ELSE '10'
END
FROM "mytable";

Returns:

1
[NULL]
3
4.5
10
10

The strings for '1' and '10' got cast into numbers. id is still an integer/number. The decimal or float of 4.5 got to stay as is. I suppose the correct type for this would be a nullable decimal?

But if you change the query to this:

SELECT CASE
    WHEN id = 1 THEN '1'::text
    WHEN id = 2 THEN null
    WHEN id = 3 THEN id
    WHEN id = 4 THEN 4.5
    ELSE '10'
END
FROM "mytable";

You get an error: SQL Error [42804]: ERROR: CASE types text and bigint cannot be matched

or this:

SELECT CASE
    WHEN id = 1 THEN 'foobar'
    WHEN id = 2 THEN null
    WHEN id = 3 THEN id
    WHEN id = 4 THEN 4.5
    ELSE '10'
END
FROM "mytable";

You get a similar error: SQL Error [22P02]: ERROR: invalid input syntax for type numeric: "foobar"

So to sum up: I think you are on the right track, with forcing these types of queries into a type rather than an any/interface{}

I don't know how realistic these are in the real world. Perhaps we should try to find queries that hit an actual table of data, rather than case statements of literals.

I agree that it's not quite realistic, I better left it to sqlc user to decide which data type that will be used in case of multiple data type exist on case-when-then-else. In short, I'll make it interface then

Based on the tests, I believe this fixes the issue for me.
How do we get this reviewed by sqlc devs?

from my previous PR, I mentioned @kyleconroy to ask for review. would you mind checking it @kyleconroy? sorry i still can't assign reviewer for this PR

bumping this PR, is there anything more i can do for this PR?

Tried this on current main; it applies apart from a one-line whitespace hunk. CASE nullability is right now, but a WHEN result that's a column reference types as any:

create table books (id int primary key, title text not null);
create table shelves (id int primary key, label text not null, book_id int);

-- name: Shelf :many
select case when b.id > 0 then s.label else null::text end shelf
from books b left join shelves s on s.book_id = b.id;

With this PR, Shelf returns []any; on main it returns []string (wrongly non-null), and it should be []sql.NullString. convertCaseExprCondToColumn falls back to any for an *ast.ColumnRef; resolving it the way outputColumns resolves a plain column reference would keep the type.

Scalar subqueries (#4262) aren't covered here; I'll propose that fix separately.