[CALCITE-6265] Type coercion is failing for numeric values in prepared statements

[tindzk]

Given a column of type INT. When providing a short value as a placeholder in a prepared statement, a ClassCastException is thrown.

Test case

final String sql =
    "select \"empid\" from \"hr\".\"emps\" where \"empid\" in (?, ?)";
CalciteAssert.hr()
    .query(sql)
    .consumesPreparedStatement(p -> {
        p.setShort(1, (short) 100);
        p.setShort(2, (short) 110);
    })
    .returnsUnordered("empid=100", "empid=110");

Stack trace

java.lang.ClassCastException: class java.lang.Short cannot be cast to class java.lang.Integer (java.lang.Short and java.lang.Integer are in module java.base of loader 'bootstrap')
     at Baz$1$1.moveNext(Unknown Source)
     at org.apache.calcite.linq4j.Linq4j$EnumeratorIterator.<init>(Linq4j.java:679)

[asolimando]

I have added the "discussion-in-jira" label as Julian raised an interesting point, we might be looking at the symptoms but not at the cause. @tindzk, would you mind checking in Avatica if there is a way to prevent this problem from happening by coercing/casting there?

[sonarqubecloud[bot]]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[mihaibudiu]

the next step is to rebase on main and squash all the commits into a single one in preparation for merging

[asolimando]

the next step is to rebase on main and squash all the commits into a single one in preparation for merging

@tindzk please make sure the commit message is well formatted and it matches the title of the corresponding Jira ticket when you rebase, thanks!

[tindzk]

@asolimando I squashed the commits and updated the title.

[tindzk]

After checking the referenced code I am not sure whether the issue can be fixed in Avatica. In order to perform type coercion, the target type needs to be known which does not seem to be the available there. Without this information, we also cannot check for overflows.

Note that type conversions are already happening in RexToLixTranslator as part of the EnumUtils.convert() call. The main contribution of my PR is to correctly convert numeric types to the expected target type. Currently, these conversions are failing due to a Java limitation that prevents numeric object types (java.lang.Long etc.) from being directly cast to another numeric type. This is solved by first deriving a primitive long (not Long) value that can then be safely cast to the target type. This indirection does require an additional check because the object on which longValue() gets called may be null. I have not been able to find a simpler workaround for this Java limitation.

The remaining changes are unrelated to the reported issue and fix tests that were requested during the PR review, notably passing values to numeric(N) placeholders, and preventing overflows that could arise in the conversion process above. I also expect that convertChecked() will be replaced by Mihai's implementation after https://github.com/apache/calcite/pull/3589 has been merged which will increase the type coverage.

[mihaibudiu]

From my understanding of the codebase this is not an Avatica issue.

[mihaibudiu]

It would be nice to get this in for the next release, so hopefully we can merge this.

[mihaibudiu]

From my understanding of the codebase this is not an Avatica issue.

Maybe I am wrong. I guess this is a bug in the transfer of data between JDBC and the evaluator, and JDBC is the one that should enforce the constraints given by the type. I haven't looked at the flow of data enough to judge this.

[asolimando]

It would be nice to get this in for the next release, so hopefully we can merge this.

I agree, that's why I have set the fixVersion to 1.37.0 in Jira days ago. We still have a bit of time for that.

After checking the referenced code I am not sure whether the issue can be fixed in Avatica. In order to perform type coercion, the target type needs to be known which does not seem to be the available there. Without this information, we also cannot check for overflows.

Indeed after a closer look I agree that we don't seem to have enough context, neither in the Avatica setters I suggested earlier, nor in Calcite at an earlier stage, or at least I could not figure it out with the time I could devote.

So yeah, not sure what Julian had in mind in his Jira comment, could be worth replying there with our findings and at least give him a chance to clarify, if there are no objections or replies for a while, then we merge what we have, we don't need to lose the release train.

WDYT?