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

tindzk · 2024-02-14T16:56:52Z

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)

mihaibudiu · 2024-02-14T18:21:13Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

@@ -1374,11 +1374,35 @@ private Result toInnerStorageType(Result result, Type storageType) {
    }
    final Type storageType = currentStorageType != null
        ? currentStorageType : typeFactory.getJavaClass(dynamicParam.getType());
-    final Expression valueExpression =
+
+    // For numeric types, get the value using the following functions on the


This may work for numeric values, but does not seem to be a general solution for other types. Is there a design that would handle that? It looks like any source/destination type combination is possible. So perhaps this could be handled like a cast expression? Although I don't see the destination type in this function.

Indeed. The PR only fixes the issue for numeric values, but for other types the user would still encounter a ClassCastException. Ideally, we would have a function that handles all implicit conversions and fails with a user-friendly error for unsupported conversions. I am sure this functionality already exists in the Calcite in the form of CAST expressions. Possibly it could be leveraged here.

As @mihaibudiu suggests, this is basically a cast, which is already available via the Types.castIfNecessary method, see below for how it can be used:

final boolean isNumeric = SqlTypeFamily.NUMERIC.contains(dynamicParam.getType()); final Expression valueExpression = Types.castIfNecessary(storageType, EnumUtils.convert(Expressions.call(root, BuiltInMethod.DATA_CONTEXT_GET.method, Expressions.constant("?" + dynamicParam.getIndex())), isNumeric ? java.lang.Number.class : storageType));

If you replace your code with this, it passes all the tests you added, and it might be working for non-numeric cases too (maybe with some adaptations).

Could you give it a try and add more tests non-numeric types to check?

Thanks for the suggestion! It greatly simplifies the fix.

I made the requested change, but kept the null check as the cast operation may fail. For instance, o.intValue() requires o to be non-null. Otherwise, testPreparedStatement would fail.

As for additional tests, do you have in mind assigning a non-numeric value to a numeric value? These would still raise an exception:

@Test void bindStringParameter() { for (SqlTypeName tpe : SqlTypeName.INT_TYPES) { final String sql = "with cte as (select cast(100 as " + tpe.getName() + ") as empid)" + "select * from cte where empid = ?"; CalciteAssert.hr() .query(sql) .consumesPreparedStatement(p -> { p.setString(1, "100"); }) .returnsUnordered("EMPID=100"); } }

This throws a ClassCastException which we could rewrite into a more user-friendly message. Alternatively, we could prevent the conversion happening in the first place by introducing a validation check.

java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Number

I personally would file a new Jira issue for the casts not covered and merge this PR as is.
You have to decide which casts can appear in this context.

Agreed. It seems unrelated to this PR. I created a ticket for it: https://issues.apache.org/jira/browse/CALCITE-6284

Sorry I haven't been very precise, I didn't mean to cast Strings to numbers, what I had in mind was values from the string family to be used in parameters of the same family, possibly with (e.g., varchar(2) value for varchar(3) parameter), but I don't know if it is even expressible with the APIs we use to pass literals in, so feel free to leave this outside the scope of the present PR in case.

I made the requested change, but kept the null check as the cast operation may fail. For instance, o.intValue() requires o to be non-null. Otherwise, testPreparedStatement would fail.

I might have made a mistake but I tried all your tests with the suggested piece of code and all passed.
Did you add a new test specifically for this maybe?

Yes, that makes sense. I added a similar test based on Mihai's request below. Since this PR is mostly about numeric types, VARCHAR conversions could be tackled in a separate ticket.

Indeed. All the tests I added were passing with your change, but testPreparedStatement existed before and started failing. Explicitly checking whether the value is null, fixed it.

core/src/test/java/org/apache/calcite/test/JdbcTest.java

asolimando · 2024-02-14T22:38:03Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

@@ -1374,11 +1374,35 @@ private Result toInnerStorageType(Result result, Type storageType) {
    }
    final Type storageType = currentStorageType != null
        ? currentStorageType : typeFactory.getJavaClass(dynamicParam.getType());
-    final Expression valueExpression =
+
+    // For numeric types, get the value using the following functions on the


As @mihaibudiu suggests, this is basically a cast, which is already available via the Types.castIfNecessary method, see below for how it can be used:

final boolean isNumeric = SqlTypeFamily.NUMERIC.contains(dynamicParam.getType()); final Expression valueExpression = Types.castIfNecessary(storageType, EnumUtils.convert(Expressions.call(root, BuiltInMethod.DATA_CONTEXT_GET.method, Expressions.constant("?" + dynamicParam.getIndex())), isNumeric ? java.lang.Number.class : storageType));

If you replace your code with this, it passes all the tests you added, and it might be working for non-numeric cases too (maybe with some adaptations).

Could you give it a try and add more tests non-numeric types to check?

asolimando · 2024-02-15T19:29:07Z

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?

mihaibudiu · 2024-02-26T22:04:52Z

core/src/test/java/org/apache/calcite/test/JdbcTest.java

+    }
+  }
+
+  @Test void bindLongParameter() {


Can you also add a test where the value overflows the type, e.g., TINYINT with a value of 300?

TINYINT(300) is not valid SQL, but while working on a test for it, I noticed that NUMERIC(<digits>) conversions are currently not supported either. An exception is thrown because Primitive.ofBox(returnType)) returns null for BigDecimal. This is fixed in the last commit.

CAST(300 as TINYINT)

Thanks for clarifying. CAST(300 as TINYINT) is correctly fine, but CAST(? as TINYINT) does not trigger an error when the value is out of bounds. I added a test and fixed it.

mihaibudiu · 2024-02-29T04:58:27Z

core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java

-            storageType);
+            isNumeric ? java.lang.Number.class : storageType);
+
+    // The cast operation will fail for null values


I can see why this comment may be confusing: what you mean is that casts have to be handled specially, because otherwise they would cause a failure. But the comment as phrased can be interpreted that a cast null will cause a failure.

I clarified the comment.

mihaibudiu · 2024-02-29T05:13:08Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java

      Type type) {
-    throw Extensions.todo();
+    if (type == Byte.class


It's funny, I implemented this myself in a separate PR: #3589
There I am relying on an existing implementation in Primitive.numberValue.

Nice. Your approach seems to cover more scenarios.

I will keep convertChecked as-is for now. The inlined validations should be sufficient within the current PR's scope, but feel free to replace it once #3589 has been merged.

mihaibudiu · 2024-02-29T05:15:32Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java

+  /**
+   * Creates an expression that represents the throwing of an exception.
+   */
+  public static Expression throwExpr(Expression expression) {


There's a ThrowStatement.
Maybe having an expression is useful, since it can be used in an expression context - you don't need to break the current expression to create a new statement.
I would document this feature.

throwExpr() is perhaps overly specific. I generalised the function such that it can be used with any statement.

asolimando

The PR is getting to a mergeable status, but we need tests covering the new/modified expressions in ExpressionTest.

Could you also check Sonar issues and fix the ones which are legit?

core/src/test/java/org/apache/calcite/test/JdbcTest.java

mihaibudiu · 2024-02-29T22:40:28Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/CastExpression.java

+/**
+ * Represents an expression cast to the specified type.
+ */
+public class CastExpression extends Expression {


I thought I left a comment in my previous review that the functionality of Cast already seems to exist: it is a UnaryExpression with type Convert. I don't think we should have two ways to do the same thing unless you can explain clearly how this is different; this will be very hard to maintain, since people won't know where to look for the functionality of casts. Why can't you reuse the Convert?
For some reason I don't see this comment, maybe I forgot to type it.

Agree, this was also suggested in #3687 (comment) and adopted in a following commit, so the change was intentional I guess, but if possible let's reuse convert

Thanks for pointing it out. I was not aware that Convert maps onto a cast operation. I removed CastExpression and changed the code to use Expressions.convert_() instead.

sonarcloud · 2024-03-06T10:19:05Z

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

asolimando

LGTM, nice work @tindzk!

mihaibudiu · 2024-03-07T05:42:40Z

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

asolimando · 2024-03-07T07:59:20Z

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!

…d statements 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) ```

tindzk · 2024-03-07T11:44:28Z

@asolimando I squashed the commits and updated the title.

asolimando

@tindzk by checking the Jira ticket I just remembered we didn't follow up on Julian's suggestions to look into the Avatica side before trying to fix it into the Calcite side.

Not surprisingly he seems to be right: if you check AvaticaSite.java#L93, you have setXYZ methods, where we have all the information we need to check overflows and possibly do type coercion.

It seems the right thing to do for two reasons:

it's early on, the earliest we fix this kind of issues, the better it is
kind of a consequence of 1), but the fix in Linq realms seems overly complicated compared to what we can do on the Avatica side, this suggests that we should do it before landing there

For these reasons I am removing my approval and I am asking you to check the aforementioned AvaticaSide and see if we can get a simpler fix there.

If after this due diligence we are still convinced that Calcite (Linq) is the better place to have this handled, let's make our point in Jira and try to have consensus there.

Apologies for forgetting to follow-up on Julian's message, I have also put the "discussion-in-jira" label as a reminder but it has slipped through the cracks nonetheless!

tindzk · 2024-03-08T14:41:56Z

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 #3589 has been merged which will increase the type coverage.

mihaibudiu · 2024-03-08T19:37:40Z

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

mihaibudiu · 2024-03-08T19:38:16Z

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

mihaibudiu · 2024-03-08T19:43:39Z

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 · 2024-03-11T18:43:05Z

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?

asolimando

(removing the request for changes as my questions have been answered)

Questions have been answered

mihaibudiu reviewed Feb 14, 2024

View reviewed changes

asolimando requested changes Feb 14, 2024

View reviewed changes

asolimando added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Feb 15, 2024

tindzk force-pushed the fix/CALCITE-6265 branch from 5809e02 to 1718aa0 Compare February 26, 2024 10:10

mihaibudiu reviewed Feb 26, 2024

View reviewed changes

tindzk force-pushed the fix/CALCITE-6265 branch from 1718aa0 to 5506fd9 Compare February 27, 2024 09:36

mihaibudiu reviewed Feb 29, 2024

View reviewed changes

asolimando requested changes Feb 29, 2024

View reviewed changes

core/src/test/java/org/apache/calcite/test/JdbcTest.java Outdated Show resolved Hide resolved

mihaibudiu reviewed Feb 29, 2024

View reviewed changes

asolimando approved these changes Mar 6, 2024

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Mar 7, 2024

tindzk changed the title ~~[CALCITE-6265] Allow providing different numeric type as placeholder~~ [CALCITE-6265] Type coercion is failing for numeric values in prepared statements Mar 7, 2024

tindzk force-pushed the fix/CALCITE-6265 branch from 5ab7e9c to 50f208d Compare March 7, 2024 11:39

asolimando previously requested changes Mar 7, 2024

View reviewed changes

asolimando removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Mar 7, 2024

asolimando reviewed Mar 11, 2024

View reviewed changes

asolimando self-requested a review March 11, 2024 18:46

mihaibudiu merged commit 4e6a320 into apache:main Apr 8, 2024
15 of 16 checks passed

tindzk deleted the fix/CALCITE-6265 branch April 9, 2024 08:01

rubenada mentioned this pull request Apr 12, 2024

[CALCITE-6265] Type coercion is failing for numeric values in prepared statements (follow-up) #3758

Merged

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

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

Conversation

tindzk commented Feb 14, 2024 • edited

Test case

Stack trace

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asolimando commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asolimando left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 6, 2024

Quality Gate passed

asolimando left a comment

Choose a reason for hiding this comment

mihaibudiu commented Mar 7, 2024

asolimando commented Mar 7, 2024

tindzk commented Mar 7, 2024

asolimando left a comment

Choose a reason for hiding this comment

tindzk commented Mar 8, 2024

mihaibudiu commented Mar 8, 2024

mihaibudiu commented Mar 8, 2024

mihaibudiu commented Mar 8, 2024

asolimando commented Mar 11, 2024

asolimando left a comment

Choose a reason for hiding this comment

tindzk commented Feb 14, 2024 •

edited

asolimando left a comment •

edited