Jooq: VALUES constructor generates wrong SQL if one row contains a null bind variable

[ciaccia]

Consider the following table (in Postgresql)

create table jooq_bug
(
    id bigint PRIMARY KEY,
    prop text,
    ts timestamp with time zone
);

Then define a Row3 CTE and insert the data into the above table:

Row3<Long, String, Timestamp> rows[] = new Row3[2];
rows[0] = DSL.row(10L, "Bar", new Timestamp(10));
rows[1] = DSL.row(10L, "Foo", null);
CommonTableExpression<Record3<Long, String, Timestamp>> cte =
    DSL.name("cte").fields("id", "prop", "ts").as(DSL.selectFrom(DSL.values(rows)));

InsertOnDuplicateStep<JooqBugRecord> step = dslCtx
    .with(cte)
    .insertInto(JooqBug.JOOQ_BUG, JooqBug.ID, JooqBug.PROP, JooqBug.TS)
    .select(DSL.select(
        cte.field("id", Long.class),
        cte.field("prop", String.class),
        cte.field("ts", Timestamp.class)
    ).from(cte)
);
System.out.println(step);
int inserted = step.execute();

step.execute() will throw the following exception:

ERROR: VALUES types timestamp without time zone and character varying cannot be matched
    Position: 126
        at org.jooq_3.9.1.POSTGRES.debug(Unknown Source)
        at org.jooq.impl.Tools.translate(Tools.java:1983)
        ...

The problem here is that while System.out above prints a valid SQL, the SQL generated during execution is wrong.

This is the System.out print (which is valid)

with "cte"("id", "prop", "ts") as (
  select 
    "v"."c1", 
    "v"."c2", 
    "v"."c3"
  from (values(10, 'Bar', timestamp '1970-01-01 01:00:00.01'),
              (10, 'Foo', null)) as "v"("c1", "c2", "c3")
)
insert into "public"."jooq_bug" (
  "id", 
  "prop", 
  "ts"
)
select 
  "cte"."id", 
  "cte"."prop", 
  "cte"."ts"
from "cte"

This is the SQL generated when executing the statement (which is wrong):

with "cte"("id", "prop", "ts") as (
  select 
    "v"."c1", 
    "v"."c2", 
    "v"."c3"
  from (values(?, ?, cast(? as timestamp)),
              (?, ?, cast(? as varchar))) as "v"("c1", "c2", "c3")
)
insert into "public"."jooq_bug" (
  "id", 
  "prop", 
  "ts"
)
select 
  "cte"."id", 
  "cte"."prop", 
  "cte"."ts"
from "cte"

The cast(? as varchar) inside the CTE in the second SQL is wrong and generates the error.

[lukaseder]

Thank you very much for reporting this in detail. In fact, it has nothing to do with the CTE, but with the VALUES() constructor implementation in jOOQ. (or if you take it one step further, the problem is really the generated cast. see also: https://github.com/jOOQ/jOOQ/issues/1735) I can reproduce the same issue more easily as such:

Result<Record4<Integer, Integer, String, Date>> result = ctx
    .selectFrom(values(
        row(1, 1, "a", Date.valueOf("2017-01-01")),
        row(2, null, null, null)))
    .fetch();

I wonder if it's really possible to coerce a row type on each individual row.

Will think abou this more thoroughly.

[akinfermo]

Has this gotten fixed? Thanks!

[lukaseder]

@akinfermo : Not yet :-)

[akinfermo]

@lukaseder Here's a fix that I've used. I simply ignore null values and their corresponding fields when building my insert statement. That way JOOQ doesn't need to infer what type to cast "null" to.

[lukaseder]

Yes you could do that. Or you could wrap all values explicitly in DSL.val(value, datatype) calls

[ciaccia]

This problem also "exists" in SQL, and Postgres uses the same rules used for UNION when resolving the column types.

For example, in Postgres the following SQL returns both columns having an integer type despite the first column being text:

select * from (values (10, '20'), (20, 30)) as "t"("id", "val")

The problem here is that (at least to my knowledge) the single rows are processed/bound one at a time rather than all together. Theoretically jOOQ should first compute the type for the columns using the rules above and then process the single rows always using the same types for all the rows.

When I write something like this, in jOOQ 3.11.2 DSL.valuesreturns an instance of org.jooq.impl.Values which just contains an array of rows without any information about the resolved column types:

Table<Record2<Integer, String>> values = DSL.values(DSL.row(10, "bar"), DSL.row(20, null));

Another possibility would be to add a new API to create "typed" values:

Table<Record2<Integer, String>> values = DSL.values(DSL.columns(Long.class, String.class), DSL.row(10, "bar"), DSL.row(20, null));

(In this case the resolution algorithm could be avoided)
Then the resolved column types should be propagated to the rendering when the variables are bound. I'm not saying this is an easy change, just proposing a possible solution to this problem 😇

[lukaseder]

PostgreSQL has a funky syntax where the derived column list can provide data type definitions in some cases:

select * from function() as "t"("id" int, "val" text)

Unfortunately, this doesn't work with arbitrary tables, only with functions. That doesn't matter for jOOQ, which can bend the rules and emulate things using derived tables and explicit casts. The relevant feature request is here: #5926

The workaround is still viable. Wrap all null values in a DSL.val() with explicit data type, but I definitely think that this data type should be inferred by org.jooq.impl.Values if it is absent, and if inference is non-ambiguous (i.e. all values of the same column are of the same type, except the null values).

I'll look into this right now.

[lukaseder]

One solution would be to patch the constructor in Values with this rather complicated set of loops:

    private void init() {
        boolean rewrite = false;

        // Find all the first well known types in the VALUES expression
        for (int i = 0, set = 0; i < rows.length && (set < rows.length || !rewrite); i++) {
            DataType<?>[] t = rows[i].dataTypes();

            for (int j = 0; j < t.length; j++) {
                if (t[j].getType() == Object.class) {
                    rewrite = true;
                }
                else if (types[j] == null) {
                    set++;
                    types[j] = t[j];
                }
            }
        }

        // If any unknown types are present, rewrite the VALUES expression to the known types
        if (rewrite) {
            for (int i = 0; i < rows.length; i++) {
                Field<?>[] fields = null;

                for (int j = 0; j < types.length; j++) {
                    if (types[j] != null && rows[i].type(j) == Object.class) {
                        if (fields == null)
                            fields = new Field[types.length];

                        Field<?> f = rows[i].field(i);
                        if (f instanceof Val)
                            fields[j] = new Val(((Val) f).getValue(), types[j], ((Val) f).getParamName());
                    }
                }

                if (fields != null) {
                    for (int j = 0; j < fields.length; j++)
                        if (fields[j] == null)
                            fields[j] = rows[i].field(j);

                    rows[i] = DSL.row(fields);
                }
            }
        }
    }

I'm not happy with this solution, it feels not clean enough, and too specific for this problem - especially given that not all databases have the current problem. In some databases, having nulls in VALUES() (or its emulations) is totally fine. Inlining null values is also fine even in PostgreSQL. And the fix doesn't solve this problem yet in case the null value is in a correlated subquery inside of VALUES().

This definitely needs a more thorough solution, perhaps introducing new features similar to DSL.coerce()

[akinfermo]

@lukaseder Thanks for your help. Keep up the great work with JOOQ!

[ciaccia]

Hi @lukaseder, I agree with you that this solution is not very clean (since the algorithm to resolve the types is Postgres specific and not an SQL standard). Maybe the right solution is to just remove cast(? as xxx) for null values inside VALUES (at least in Postgres) since Postgres already knows what it is doing...

[lukaseder]

(since the algorithm to resolve the types is Postgres specific and not an SQL standard)

Well, the algorithm would work on all databases...

since Postgres already knows what it is doing...

You'd wish! There are many cases where bind variable types can be inferred by a variety of databases (inference cleverness varies greatly). In PostgreSQL, a lot of cases it does work, but often it does not. One example I remember is: You'd think that ?::text[] and ?::varchar(x)[] should be interchangeable, since in many cases, ?::text and ?::varchar are interchangeable. Unfortunately, this isn't true. There are many edge cases...

I wish to remove these casts as well, see this issue for details:
https://github.com/jOOQ/jOOQ/issues/2823

But there might be quite a few edge cases not covered by the integration tests. We'll see. Ideally, a setting would allow to revert to the current logic.

[ciaccia]

Hi @lukaseder, maybe I was not clear enough... I would remove the cast(? as xxx) for null values inside VALUES() ONLY (since values is not "typed", i.e. we cannot give a column type in the column definition).

I would argue that this (at least in Postgres) cannot create any problem for the following reasons:

• Rows inside VALUES() are "typeless" (i.e. types are determined with their algorithm)
• Having a null value has no impact on anything (since we are not calling a function where the signature must match etc.)
• The problem with non interchangeable types don't happen for a null without type since no type is provided (and at the end the type will be unknown).

Or am I missing something?

[lukaseder]

And me too, I was not clear enough. :-) Removing the cast(? as xxx) expressions inside of some other expression is not at all simple.

1. VALUES() wraps Row. Should we continue to do that (profiting from some code reuse) or duplicate the row SQL generation code into values, patching it there?
2. There's no reason why we wouldn't put an expression in VALUES(). Say VALUES(?, ?+?, foo(?)+bar(?), (select ?)). Are these expressions still inside of VALUES(), i.e. does your optimisation still apply?

Or am I missing something?

Try this in Postgres:

select a + b from (values(null, 1)) t(a, b)

You'll get:

SQL Error [42883]: ERROR: operator does not exist: text + integer

So, while some automatic type casting may happen in the presence of at least one row that helps define the type (see below), it doesn't always happen. E.g. this works:

select a + b from (values(1, null),(null, 1)) t(a, b)

The thorough solution in the presence of nulls is to pass the type information to jOOQ as I've mentioned before. A thorough solution inside of jOOQ is quite difficult and would be similar to what I've suggested, except that the implementation I've suggested is located at the wrong spot.

[garthgoodson]

I'm on an older Jooq Version (3.8.9) and see something similar but not involving values(). Something like:

      Record r = db.select(DSL.max(tsField).as("max"), DSL.min(tsField))
            .from(tmpTable)
            .fetchOne();

      Select subQuery = ....
            .where(ACCOUNT_METRICS.TIMESTAMP.between(r.getValue("min", Long.class))
                                                     .and(r.getValue("max", Long.class)))
            .orderBy(ACCOUNT_METRICS.KEY)...

The where clause is generating similar code as discussed above:
where "account_metrics"."timestamp" between ? and cast( ? as varchar)

[lukaseder]

Thanks for your message, @garthgoodson. GitHub wisely stores issue IDs as BIGINT, which means we will not run out of issue IDs any time soon. May I ask you to create a new issue for this? :-)

[lukaseder]

Ran into this myself in integration tests, just now. Will look into a fix now, but the workaround is simple, too. Use explicit DSL.val(Object, Class<T>) calls, whenever null values are present:

Row3<Long, String, Timestamp> rows[] = new Row3[2];
rows[0] = DSL.row(10L, "Bar", new Timestamp(10));
rows[1] = DSL.row(DSL.val(10L), DSL.val("Foo"), DSL.val(null, Timestamp.class));

[lukaseder]

Similar to what has been discovered here: https://github.com/jOOQ/jOOQ/issues/10312#issuecomment-651143792, we need a way to create lazy type inference for bind values in various places.

Currently, when a bind value is wrapped in DSL.val(), the data type is inferred eagerly from context:

1. DSL.val(Object) -> Using instanceof. If null, tough luck
2. DSL.val(String) -> Using a hard coded type, e.g. SQLDataType.VARCHAR
3. DSL.val(Object, DataType) -> Using the argument data type if available (this is used e.g. when writing Field<T>.eq(T), where the lhs's data type is used to wrap the bind value, or also Row2<T1, T2>.eq(T1, T2)

This technique 3) should be used here as well, but lazily. In the absence of any better type information at wrap-time, we have to use SQLDataType.OTHER as in case 1), but the fact that this OTHER type is somehow again wrapped in a values() constructor means that the values() constructor can override it. https://github.com/jOOQ/jOOQ/issues/10312#issuecomment-651143792 is about doing the same thing when someone uses DSL.val(T) or DSL.inline(T) explicitly, rather than letting jOOQ wrap the thing as in 3).

I'll try to fix this for jOOQ 3.14, as it will resolve a few subtle issues.

[lukaseder]

It seems this problem gets fixed implicitly if we simply stop casting the OTHER type to VARCHAR: https://github.com/jOOQ/jOOQ/issues/11511

[lukaseder]

Precision: It gets fixed for PostgreSQL, not for all dialects. There still needs to be a fix for other dialects.

[lukaseder]

There's a surprising regression in Firebird's emulation of this clause, where a NULL bind value, thus far, was cast to VARCHAR and no longer is:

Result<Record4<Integer, Integer, String, Date>> result = create()
    .selectFrom(values(
        row(1, 1, "a", Date.valueOf("2017-01-01")),
        row(2, null, null, null)))
    .fetch();

Resulting in:

select "v"."c1", "v"."c2", "v"."c3", "v"."c4"
from (
  select cast(? as integer), cast(? as integer), cast(? as varchar(1)), cast(? as date) 
  from RDB$DATABASE 
  union all 
  select cast(? as integer), ?, ?, ? 
  from RDB$DATABASE
) "v" ("c1", "c2", "c3", "c4")

The surprise here is that casting the bind values to varchar didn't cause any trouble in Firebird so far. Of course, we should make sure that all rows are coerced to the same set of types, irrespective of where the nulls appear.

[lukaseder]

With this depending on a rather delicate change (#11511), I won't backport this fix to 3.14

[lukaseder]

Fixed in jOOQ 3.15.0