calcite icon indicating copy to clipboard operation
calcite copied to clipboard

[CALCITE-3100] cast(? as DATE) won't work with PreparedStatement

Open yuqi1129 opened this issue 6 years ago • 7 comments

Fix cast(? as DATE) won't work with PreparedStatement bug

This is a fix about the problem when when PreparedStatement with DynamicParam. for example

PreparedStatement ps = connection.prepareStatement("select * from EMPS where JOINEDAT=cast(? as DATE)");
ps.setString(1, "1996-08-03");
ps.execute();

The above problem will cause exception. the PR is a fix for this probme. for more, you can refer to issue 3100

yuqi1129 avatar Jul 05 '19 10:07 yuqi1129

hmm.. I think this CALCITE-3100 is really a good catch for the shortage when using SqlDynamicParam in CAST. The core problem is that SqlValidator infers the "type" of SqlDynamicParam by the "type" of CAST. And during the optimization, such a CAST will be regarded as redundant and get simplified/peeled.

But this PR just fixes the casting to type of DATE/TIME, what about other common types? e.g. cast(String as Integer) and so on ....

In my experiment, below simple query is failing

    PreparedStatement pstmt = connection.prepareStatement("select \"name\", \"deptno\" from \"emps\" WHERE \"empid\"=cast(? as INTEGER)");
    pstmt.setString(1, "100");
    pstmt.execute();

I believe I can construct lots of more failures like above. If we only fixs the casting to type of DATE/TIME in RexToLixTranslator.java, it feels like a too special case for RexToLixTranslator.

How do you think @danny0405 ?

jinxing64 avatar Jul 06 '19 17:07 jinxing64

@yuqi1129 THX again for this fix. It will be great if you can give a short description for the main idea of this PR as your first comment.

Below comment is not friendly enough.

Fix cast(? as DATE) won't work with PreparedStatement bug

jinxing64 avatar Jul 06 '19 17:07 jinxing64

hmm.. I think this CALCITE-3100 is really a good catch for the shortage when using SqlDynamicParam in CAST. The core problem is that SqlValidator infers the "type" of SqlDynamicParam by the "type" of CAST. And during the optimization, such a CAST will be regarded as redundant and get simplified/peeled.

But this PR just fixes the casting to type of DATE/TIME, what about other common types? e.g. cast(String as Integer) and so on ....

In my experiment, below simple query is failing

    PreparedStatement pstmt = connection.prepareStatement("select \"name\", \"deptno\" from \"emps\" WHERE \"empid\"=cast(? as INTEGER)");
    pstmt.setString(1, "100");
    pstmt.execute();

I believe I can construct lots of more failures like above. If we only fixs the casting to type of DATE/TIME in RexToLixTranslator.java, it feels like a too special case for RexToLixTranslator.

How do you think @danny0405 ?

Your suggestion is impressive and meaningful, other types indeed have the same problem as date, I need to cover all cases when using SqlDynamicParam besides the date

yuqi1129 avatar Jul 08 '19 02:07 yuqi1129

@yuqi1129 THX again for this fix. It will be great if you can give a short description for the main idea of this PR as your first comment.

Below comment is not friendly enough.

Fix cast(? as DATE) won't work with PreparedStatement bug

OK, I will change the description as soon as possible

yuqi1129 avatar Jul 08 '19 02:07 yuqi1129

@jinxing64

SqlValidator infers the "type" of SqlDynamicParam by the "type" of CAST. And during the optimization, such a CAST will be regarded as redundant and get simplified/peeled.

SqlValidatorImpl did use the operand0 type of the SqlDynamicParam directly[1], and it's the sql-to-rel conversion that create a RexDynamicParam with the correct type[2] (not the optimization phrase).

We can see that the RexDynamicParam has the correct data type, but the java type factory has an int type for sql DATE and TIMESTAMP type. [3]

@yuqi1129 Although cast to string.class type first can solve this problem, i still think the intermediate type should depend on what the type the JDBC DataContext returns.

[1] https://github.com/apache/calcite/blob/046f9f4e2e3c779e4e2e61946d67a0dc124f7f72/core/src/main/java/org/apache/calcite/sql/fun/SqlCastFunction.java#L107 [2] https://github.com/apache/calcite/blob/046f9f4e2e3c779e4e2e61946d67a0dc124f7f72/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L3006 [3] https://github.com/apache/calcite/blob/046f9f4e2e3c779e4e2e61946d67a0dc124f7f72/core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java#L179

danny0405 avatar Jul 08 '19 08:07 danny0405

return type.isNullable() ? Integer.class : int.class;

@danny0405 Do you have any idea about this problem that superior than current method using 'string' as intermediate type. the following is my point, if any is wrong, do not hesitate to point out.

As far as i can see, when we use

 stringStatement.setString(1, "2");

we just take the parameter as a string value, so casting to string, i think, is reasonable. As for other method such as

intStatement.setInt(1, 2);

as the following code return a value with object type, string is most compatible

Expression expression = Expressions.call(root, BuiltInMethod.DATA_CONTEXT_GET.method,
            Expressions.constant("?" + expr.getIndex()));

yuqi1129 avatar Jul 09 '19 12:07 yuqi1129

@yuqi1129 The best way i can think of is to merge in the expressions of CAST operator, because we know the physicalType and logicalType, we may need to know what type the JDBC DataConext returns and use the cast expressions to bridge them.

danny0405 avatar Jul 10 '19 01:07 danny0405