[jira] [Comment Edited] (CALCITE-6265) Type coercion is failing for numeric values in prepared statements
[
https://issues.apache.org/jira/browse/CALCITE-6265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17836560#comment-17836560
]
Ruben Q L edited comment on CALCITE-6265 at 4/15/24 3:10 PM:
-
I have [opened a PR|https://github.com/apache/calcite/pull/3758] to fix the
problems (partially reverting some of the original changes, partially making
some adjustments):
- It seems the original issue could be solved easier in RexToLixTranslator, by
simply converting to Number and then to storageType in case of Numeric types.
- All the bind* tests that were added on the first PR still work; plus a new
test that I added to illustrate the issues that I found (with setBigDecimal)
- The problems that I detected on my downstream project are also fixed.
- The only thing that is missing (didn't have the time to dig deeper) from the
original solution is the "check for overflow and throw". I have left the
auxiliary method that generates this dynamic code (Expressions#convertChecked),
and left a TODO on EnumUtils#convert for future work. I think that this was not
part of the strict scope of the current ticket's description, so IMHO it would
be acceptable open a separate ticket and work on that in the future, adding
more thorough tests on this regard (and not just the one
JdbcTest#bindOverflowingTinyIntParameter that was originally added, which I
disabled on my branch). UPDATE: created CALCITE-6366 for this purpose.
I'd appreciate some feedback, and if you think I can move on and merge my
proposal.
was (Author: rubenql):
I have [opened a PR|https://github.com/apache/calcite/pull/3758] to fix the
problems (partially reverting some of the original changes, partially making
some adjustments):
- It seems the original issue could be solved easier in RexToLixTranslator, by
simply converting to Number and then to storageType in case of Numeric types.
- All the bind* tests that were added on the first PR still work; plus a new
test that I added to illustrate the issues that I found (with setBigDecimal)
- The problems that I detected on my downstream project are also fixed.
- The only thing that is missing (didn't have the time to dig deeper) from the
original solution is the "check for overflow and throw". I have left the
auxiliary method that generates this dynamic code (Expressions#convertChecked),
and left a TODO on EnumUtils#convert for future work. I think that this was not
part of the strict scope of the current ticket's description, so IMHO it would
be acceptable open a separate ticket and work on that in the future, adding
more thorough tests on this regard (and not just the one
JdbcTest#bindOverflowingTinyIntParameter that was originally added, which I
disabled on my branch).
I'd appreciate some feedback, and if you think I can move on and merge my
proposal.
> 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: Ruben Q L
>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.(Linq4j.java:679)}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Comment Edited] (CALCITE-6265) Type coercion is failing for numeric values in prepared statements
[
https://issues.apache.org/jira/browse/CALCITE-6265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17836560#comment-17836560
]
Ruben Q L edited comment on CALCITE-6265 at 4/12/24 11:16 AM:
--
I have [opened a PR|https://github.com/apache/calcite/pull/3758] to fix the
problems (partially reverting some of the original changes, partially making
some adjustments):
- It seems the original issue could be solved easier in RexToLixTranslator, by
simply converting to Number and then to storageType in case of Numeric types.
- All the bind* tests that were added on the first PR still work; plus a new
test that I added to illustrate the issues that I found (with setBigDecimal)
- The problems that I detected on my downstream project are also fixed.
- The only thing that is missing (didn't have the time to dig deeper) from the
original solution is the "check for overflow and throw". I have left the
auxiliary method that generates this dynamic code (Expressions#convertChecked),
and left a TODO on EnumUtils#convert for future work. I think that this was not
part of the strict scope of the current ticket's description, so IMHO it would
be acceptable open a separate ticket and work on that in the future, adding
more thorough tests on this regard (and not just the one
JdbcTest#bindOverflowingTinyIntParameter that was originally added, which I
disabled on my branch).
I'd appreciate some feedback, and if you think I can move on and merge my
proposal.
was (Author: rubenql):
I have [opened a PR|https://github.com/apache/calcite/pull/3758] to fix the
problems (partially reverting some of the original changes, partially making
some adjustments):
- It seems the original issue could be solved easier in RexToLixTranslator, by
simply converting to Number and then to storageType in case of Numeric types.
- All the bind* tests that were added on the first PR still work; plus a new
test that I added to illustrate the issues that I found (with setBigDecimal)
- The problems that I detected on my downstream project are also fixed.
- The only thing that is missing (didn't have the time to dig deeper) from the
original solution is the "check for overflow and throw". I have left the
auxiliary method that generates this dynamic code, and left a TODO on
EnumUtils#convert for future work. I think that this was not part of the strict
scope of the current ticket's description, so IMHO it would be ok open a
separate ticket and work on that in the future, adding more thorough tests (and
not just the one that was originally added, which I disabled on my branch).
I'd appreciate some feedback, and if you think I can move on and merge my
proposal.
> 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.(Linq4j.java:679)}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Comment Edited] (CALCITE-6265) Type coercion is failing for numeric values in prepared statements
[
https://issues.apache.org/jira/browse/CALCITE-6265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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.45799985");}}
- 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.45799985");}}
- 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
[jira] [Comment Edited] (CALCITE-6265) Type coercion is failing for numeric values in prepared statements
[
https://issues.apache.org/jira/browse/CALCITE-6265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17836187#comment-17836187
]
Ruben Q L edited comment on CALCITE-6265 at 4/11/24 1:47 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.45799985");}}
- 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].
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.45799985");}}
- 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
[jira] [Comment Edited] (CALCITE-6265) Type coercion is failing for numeric values in prepared statements
[
https://issues.apache.org/jira/browse/CALCITE-6265?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17836187#comment-17836187
]
Ruben Q L edited comment on CALCITE-6265 at 4/11/24 1:45 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.45799985");}}
- 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).
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.45799985");}}
- A simple query: SELECT t.id FROM myTable t WHERE t.myDecimalValue = ?
- 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
