fix(postgresql): set query result column nullable if selected column contains json operator or null value in case expression

[swallowstalker]

Trying to solve #3710

JSON traversal operator on selected columns

Based on postgresql docs, operator ->> and #>> should return string, and since this is JSON and it could have null value on its field, it should return pgtype.Text. Other JSON operator should return interface{} since it returns JSON object

Refactor AExpr and AConst on selected columns

Since it is used on CaseExpr, I refactored both of them to allow it to be reusable in different location

TypeCast null value checking

If there is any selected columns which has null value to be casted to other type, for example null::text then it should return pgtype.Text to allow nullable text

CaseExpr nullable + column type handling

Previously, sqlc determines column type from default case and uses hardcoded NotNull to true. Relying on default case can be bad since it might not be declared, if user decides not to use ELSE keyword.

This PR will determine column type + nullability from all sub-case, on list of THEN clause combined with ELSE clause considering data types involved on those multiple clause.

1. If there is only one data type on any case expr clause, then it will be nullable of that data type
2. If there are multiple data type, it will automatically generate interface{} type
3. If all case expr clause have null result, then it will generate interface{} type
4. Case expr without default clause, will be assumed to have null as its ELSE result

and if any of those column are nullable, then the resulting column will be set to nullable.

[veqryn]

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";

[veqryn]

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?

[swallowstalker]

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? 👀

[swallowstalker]

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?

[veqryn]

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.

[swallowstalker]

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

[swallowstalker]

this is new addition to handle null const value. I need to distinguish between any and null, because null in CaseExpr would determine query column result nullability, but does not make it included in column type decision.

for ast.A_Const condition, i have converted DataType null back to any (line 136, outputColumns function) because we don't know its type and to preserve backward compatibility

[swallowstalker]

missing ELSE clause on CaseExpr is considered to return null value

[veqryn]

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

[swallowstalker]

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