[ https://issues.apache.org/jira/browse/CALCITE-6265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17836187#comment-17836187 ]
Ruben Q L edited comment on CALCITE-6265 at 4/11/24 2:06 PM: ------------------------------------------------------------- FYI, this fix caused a few regressions on my project: A) one test that used to return 1 row now returns 0 rows B) a couple of tests that used to work fine now fail with a ClassCastException on the dynamic code After some investigation, I'd say A seems a real regression on Calcite, whereas B could be arguably on my side; but I share this info because probably both could happen to other projects if we release the next version with this patch as it is. A) The problem with the test that no longer returns the expected 1 row is as follows: - A table with a decimal field, there is one column with value {{BigDecimal DECIMAL1 = new BigDecimal("299792.457999999985");}} - A simple query: SELECT t.id FROM myTable t WHERE t.myDecimalField = ? - I set the parameter value to DECIMAL1 - Execute the query, should get 1 row, it gets nothing. The problem is that, before the patch, the dynamic code would simply "read" the parameter value from the root context and use it, [but now with the new code there is this cast logic into Number and back to BigDecimal|https://github.com/apache/calcite/blob/e7c1f0c9baecef62289facf9aaa03bdff950b938/core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java#L1396] (unnecessary in this case, since everything is BigDecimal), which leads to the following dynamic code: {code} (Number) root.get("?1") == null ? (java.math.BigDecimal) null : java.math.BigDecimal.valueOf(((Number) root.get("?1")).longValue()) {code} The problem is that doing {{BigDecimal.valueOf(myBigDecimal.longValue())}} does not necessarily bring back the original value of myBigDecimal, since BigDecimal#longValue can lose information, and I guess this is what is happening here. B) The issue with the ClassCastException (BigDecimal cannot be cast to Integer) is also related to the new casting in dynamic code, but it is a bit more subtle, and it seems related to some internal Hoisting that we have in place on my project (transforming literals into parameters to increase the chances of internal cache hits when converting a logical plan into physical plan). The thing is, in the literal=>parameter conversion, we use [RexLiteral#getValue3|https://github.com/apache/calcite/blob/e7c1f0c9baecef62289facf9aaa03bdff950b938/core/src/main/java/org/apache/calcite/rex/RexLiteral.java#L989] (which as stated on its javadoc returns "the value of this literal, in the form that the rex-to-lix translator wants it") to extract the value (and use it as parameter value). So far it worked like charm. But with this change, the ClassCastException arises because actually getValue3 of a RexLiteral with type = BasicSqlTypeINTEGER, returns the value as a BigDecimal (as it is stored in RexLiteral#value field), because its typeName is SqlTypeName#DECIMAL; btw this RexLiteral is created by the standard behavior that we get if we create a RexLiteral from an Integer via Calcite's [RelBuilder#literal(IntegerValue)|https://github.com/apache/calcite/blob/e7c1f0c9baecef62289facf9aaa03bdff950b938/core/src/main/java/org/apache/calcite/tools/RelBuilder.java#L481]. PS: Also, I know I'm late to the party here, but one could argue if this bug is _really_ a bug, I mean, if there is an INT field, then the user should use the appropriate [setInt method; and not setShort|https://github.com/apache/calcite-avatica/blob/275a082d364e00a9f2e9d50a4468fef0782e3c81/core/src/main/java/org/apache/calcite/avatica/AvaticaSite.java#L93]. I agree than having an exception at runtime in the dynamic code in this case is ugly; but maybe a check-and-fail could be added earlier? Or maybe it should be Avatica's responsibility to perform the necessary conversions (as [~julianhyde] suggested) ? Is this bug fixing an error on Calcite side, which only happens on a certain scenario triggered by Avatica? was (Author: rubenql): FYI, this fix caused a few regressions on my project: A) one test that used to return 1 row now returns 0 rows B) a couple of tests that used to work fine now fail with a ClassCastException on the dynamic code After some investigation, I'd say A seems a real regression on Calcite, whereas B could be arguably on my side; but I share this info because probably both could happen to other projects if we release the next version with this patch as it is. A) The problem with the test that no longer returns the expected 1 row is as follows: - A table with a decimal field, there is one column with value {{BigDecimal DECIMAL1 = new BigDecimal("299792.457999999985");}} - A simple query: SELECT t.id FROM myTable t WHERE t.myDecimalField = ? - I set the parameter value to DECIMAL1 - Execute the query, should get 1 row, it gets nothing. The problem is that, before the patch, the dynamic code would simply "read" the parameter value from the root context and use it, [but now with the new code there is this cast logic into Number and back to BigDecimal|https://github.com/apache/calcite/blob/e7c1f0c9baecef62289facf9aaa03bdff950b938/core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java#L1396] (unnecessary in this case, since everything is BigDecimal), which leads to the following dynamic code: {code} (Number) root.get("?1") == null ? (java.math.BigDecimal) null : java.math.BigDecimal.valueOf(((Number) root.get("?1")).longValue()) {code} The problem is that doing {{BigDecimal.valueOf(myBigDecimal.longValue())}} does not necessarily bring back the original value of myBigDecimal, since BigDecimal#longValue can lose information, and I guess this is what is happening here. B) The issue with the ClassCastException (BigDecimal cannot be cast to Integer) is also related to the new casting in dynamic code, but it is a bit more subtle, and it seems related to some internal Hoisting that we have in place on my project (transforming literals into parameters to increase the chances of internal cache hits when converting a logical plan into physical plan). The thing is, in the literal=>parameter conversion, we use [RexLiteral#getValue3|https://github.com/apache/calcite/blob/e7c1f0c9baecef62289facf9aaa03bdff950b938/core/src/main/java/org/apache/calcite/rex/RexLiteral.java#L989] (which as stated on its javadoc returns "the value of this literal, in the form that the rex-to-lix translator wants it") to extract the value (and use it as parameter value). So far it worked like charm. But with this change, the ClassCastException arises because actually getValue3 of a RexLiteral with type = BasicSqlTypeINTEGER, returns the value as a BigDecimal (as it is stored in RexLiteral#value field), because its typeName is SqlTypeName#DECIMAL; btw this RexLiteral is created by the standard behavior that we get if we create a RexLiteral from an Integer via Calcite's [RelBuilder#literal(IntegerValue)|https://github.com/apache/calcite/blob/e7c1f0c9baecef62289facf9aaa03bdff950b938/core/src/main/java/org/apache/calcite/tools/RelBuilder.java#L481]. > Type coercion is failing for numeric values in prepared statements > ------------------------------------------------------------------ > > Key: CALCITE-6265 > URL: https://issues.apache.org/jira/browse/CALCITE-6265 > Project: Calcite > Issue Type: Bug > Components: core > Reporter: Tim Nieradzik > Assignee: Tim Nieradzik > Priority: Major > Labels: pull-request-available > Fix For: 1.37.0 > > > Given a column of type {{{}INT{}}}. When providing a {{short}} value as a > placeholder in a prepared statement, a {{ClassCastException}} is thrown. > h2. 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");}} > h2. 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)}} -- This message was sent by Atlassian Jira (v8.20.10#820010)