Types support #17

SeriyBg · 2020-12-29T13:28:12Z

Fix #6
Fix #5
Fix #18

devozerov · 2020-12-30T06:46:58Z

src/main/java/com/hazelcast/jdbc/JdbcResultSet.java

    }

    @Override
    public double getDouble(int columnIndex) throws SQLException {
-        return get(columnIndex);
+        return convertTo(get(columnIndex), Double.class);
    }

    @Override


BigDecimal with scale should be supported as well. It should be relatively trivial to implement provided that we already have conversions to BigDecimal.

devozerov · 2020-12-30T07:12:01Z

src/main/java/com/hazelcast/jdbc/TypeConverter.java

+    }
+
+    @SuppressWarnings("unchecked")
+    static <T> T convertTo(Object object, Class<T> clazz) throws SQLException {


Getting the value from the driver is a performance-sensitive operation. The current implementation looks too heavy to me, especially for simple types:

For all methods except getObject, getDate, getTime and getTimestamp, we do not need to do any lookups. It should be sufficient to do the following. It requires only one hash map lookup as opposed to three in the current impl:

static <T> T convertTo(Object object, QueryDataType type) throws SQLException { try { return type.convert(object); } catch (Exception e) { // handle exception } } }

For date, time, timestamp: first invoke the method from p.1, then apply the subsequent conversion. One lookup instead of three.

For object:

resolve the type of the passed class using QueryDataTypeUtils.resolveTypeForClass

invoke logic from p.1

if requested class is one of java.sql.Date, java.sql.Time, java.sql.Timestamp - invoke logic from p.2

check if the produced object is assignable to the requested class. If yes - return, otherwise - throw a proper exception (currently we will throw an implicit ClassCastException)

This way we have simple implementation for simple cases, and gradually make it heavier for more complex cases. We also eliminate two of three hash map lookups.

devozerov · 2020-12-30T07:13:00Z

src/main/java/com/hazelcast/jdbc/JdbcResultSet.java

@@ -43,6 +43,8 @@
 import java.util.Iterator;
 import java.util.Map;

+import static com.hazelcast.jdbc.TypeConverter.convertTo;
+
 public class JdbcResultSet implements ResultSet {


getObject(int columnIndex, Class<T> type) and getObject(String columnLabel, Class<T> type) methods ignore the passed type.

devozerov · 2020-12-30T07:15:45Z

src/main/java/com/hazelcast/jdbc/JdbcResultSet.java

@@ -43,6 +43,8 @@
 import java.util.Iterator;
 import java.util.Map;

+import static com.hazelcast.jdbc.TypeConverter.convertTo;
+
 public class JdbcResultSet implements ResultSet {


According to JDBC spec, a somewhat similar approach to conversions is required for parameters (PreparedStatement.setObject(...)). Do we have it on our radars?

Created an issue #18

devozerov · 2020-12-30T07:22:57Z

src/test/java/com/hazelcast/jdbc/DriverTypeConversionTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class DriverTypeConversionTest {


To me, the test coverage looks insufficient. Are we sure that we test every possible pair of from/to types? This is tedious, but getting the results from the ResultSet is a foundational part of the driver, and we can't allow bugs here.
I would propose to test every type pair separately with a precise contract:

If it should succeed, then what is the expected result?

If it should fail, then check that only SQLException is thrown, and check for the specific error message (since error messages are part of the public API).

We did exactly the same for the server part. See com.hazelcast.sql.impl.type.converter.ConvertersTest and com.hazelcast.sql.impl.type.converter.CastFunctionIntegrationTest as examples of this exhaustive testing approach. Also, there is a set of convenient classes to return different types of values from the query: com.hazelcast.sql.support.expressions.ExpressionTypes, com.hazelcast.sql.support.expressions.ExpressionValue.

Have added move test cases to cover from/to types pair. As well as changing the logic for conversion to the suggested one

It feels like we could benefit from the mutation testing integration.

Example: https://github.com/hazelcast/hazelcast-zookeeper/pull/60/files

Yes, definitely. I will add it as a separate PR. Also created an issue for it #19

pivovarit

Looks like @devozerov covered the most important aspects already.

src/main/java/com/hazelcast/jdbc/TypeConverter.java

pivovarit · 2020-12-31T09:01:35Z

src/test/java/com/hazelcast/jdbc/DriverTypeConversionTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class DriverTypeConversionTest {


It feels like we could benefit from the mutation testing integration.

Example: https://github.com/hazelcast/hazelcast-zookeeper/pull/60/files

devozerov · 2021-01-11T16:50:12Z

src/main/java/com/hazelcast/jdbc/JdbcResultSet.java

    }

    @Override
    public BigDecimal getBigDecimal(int columnIndex, int scale) throws SQLException {
-        throw JdbcUtils.unsupported("BigDecimal with scale is not supported");
+        return getBigDecimal(columnIndex).setScale(scale);


The setScale method may throw an ArithmeticException, that we do not wrap into an SQLException.

devozerov · 2021-01-11T17:04:54Z

src/main/java/com/hazelcast/jdbc/TypeConverter.java

+
+    @SuppressWarnings("unchecked")
+    static <T> T convertTo(Object object, QueryDataType targetDataType) throws SQLException {
+        return convertAs(


All conversion methods are on the hot path. I would avoid using lambdas here because every invocation creates a new lambda instance. It is better to have some code duplication here in favor of the speed.

Also please note that currently, we do two null checks instead of one: one check in the convertAs method, another in the QueryDataType.convert.

Another problem is that we do not consult column metadata. This forces us to call relatively heavy Converters.getConverter on the hot path. Instead, we may create a static table from SqlColumnType to QueryDataType, and use metadata to resolve the current column's type without Converters.getConverter. Also, this may lead to a situation, when, say, we have a null value for the column of TYPE_A, try to convert it to TYPE_B, and it pass. While in reality, these two types are incompatible (e.g. DATE and TIME cannot be converted to each other).

To summarize, we should do a minimal number of operations on the hot path, and make sure that we throw an exception for incompatible types even if the current value is null.

If we are going to use SqlColumnType from metadata the conversion will be much more strict. For example, having an int field with the value 42 won't be possible to convert to String, for now, it is. Is this the expected behavior?

Tested the behavior with H2, and there it is possible to call getString on DECIMAL field and conversion will be successful. And same for null values, having null as a String it is possible to successfully call getBigDecimal

H2 is a toy database, we hardly should use it as a reference. In any case, if we have a DECIMAL column, then the associated converter BigDecimalConverter allows for conversion to string (BigDecimalConverter.asVarchar).

Have tried with Postgres, and the behavior is the same: having a field of type date will null value I can successfully cal getTime which will return null. The same goes for String to BigDecimal. Should we follow the same behavior or do we decide to throw an exception for incompatible types even if the value can be converted?

In this case, we may have a null check, that return null for every combination of types, and never fail. And only if the value is not null we resort to converters.

In other words, my comment is not valid:

Also, this may lead to a situation, when, say, we have a null value for the column of TYPE_A, try to convert it to TYPE_B, and it pass. While in reality, these two types are incompatible (e.g. DATE and TIME cannot be converted to each other).

Got it. So for the null value, we will convert anyway, otherwise will convert only if it is possible. One more interesting thing for Postgres: having a null value for a String field will return 0 for the getLong call. Will follow the same behavior

devozerov · 2021-01-11T17:08:14Z

src/main/java/com/hazelcast/jdbc/TypeConverter.java

+
+    @SuppressWarnings("unchecked")
+    static <T> T convertTo(Object object, int targetSqlType) throws SQLException {
+        QueryDataType queryDataType = SQL_TYPES_TO_QUERY_DATA_TYPE.get(targetSqlType);


Why don't we support FLOAT, NUMERIC, DECIMAL, DATE, TIME, TIMESTAMP, NULL, JAVA_OBJECT, CHAR types?

AFAIK, types DATE, TIME, TIMESTAMP, NULL, JAVA_OBJECT are not supported at the moment by IMDG SQL module, please correct me if I'm wrong. So I have created the issue to add it when it will be available in sql-module: #26 and #25
For all the other types will add the implementation

They are supported in a limited way - we may get only a column of this type, but cannot use these types in expressions.

SQL_TYPES_TO_QUERY_DATA_TYPE is used for PreparedStatement, where it's not supported atm. For the ResultSet we do support these types

devozerov · 2021-01-11T17:10:38Z

src/main/java/com/hazelcast/jdbc/TypeConverter.java

+        try {
+            return supplier.get();
+        } catch (Exception e) {
+            throw new SQLException("Cannot convert '" + object + "' of type "


Many tools show only the message of the top exception. In the current implementation, the user will only see that we cannot convert one type to another, but will not see the reason. We'd better add the message of the e to the created exception.

devozerov · 2021-01-11T17:12:19Z

src/main/java/com/hazelcast/jdbc/TypeConverter.java

+    }
+
+    static Timestamp convertToTimestamp(Object object) throws SQLException {
+        return convertAs(object, () -> new Timestamp(toMillis(


convertToTimestamp, convertToTime, convertToDate - same concerns as with the convertTo method: allocations, heavy Converters.getConverter. Most of this could be avoided for a good number of cases (probably, the only exception is the source column of the OBJECT type).

SeriyBg requested review from pivovarit and devozerov December 29, 2020 13:28

devozerov reviewed Dec 30, 2020

View reviewed changes

SeriyBg requested a review from devozerov December 30, 2020 14:56

pivovarit reviewed Dec 31, 2020

View reviewed changes

SeriyBg requested a review from pivovarit January 5, 2021 17:27

SeriyBg force-pushed the types-support branch from 0ba1a1b to 4d37b4b Compare January 11, 2021 15:54

devozerov reviewed Jan 11, 2021

View reviewed changes

SeriyBg requested a review from devozerov January 15, 2021 10:17

devozerov approved these changes Jan 19, 2021

View reviewed changes

SeriyBg added 12 commits January 19, 2021 11:37

add type conversion

f5a5271

fix checkstyle issue

c750d36

fix checkstyle issue

6bccf38

add support for temporal types

9787e89

add type validation

5507661

add tests for all conversion cases

330f982

clean up

9b20994

add datetime types support and tests

8b13c62

clean up

9a1c3bb

clean up

c9284bb

clean up

1a9fc7d

fix tests

2417d9b

SeriyBg added 6 commits January 19, 2021 11:37

fix tests

f74195a

add typing support for prepared statement

ac186f3

fix part of PR comments

17db28c

fix typing and tests

4c750f0

add date/time support and nullable conversion

208dfef

fix checkstyle issues

d770e19

SeriyBg force-pushed the types-support branch from 89a2a75 to d770e19 Compare January 19, 2021 09:40

SeriyBg merged commit e81cdbf into main Jan 19, 2021

SeriyBg deleted the types-support branch January 19, 2021 09:44

SeriyBg added this to the 4.2 milestone Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Types support #17

Types support #17

SeriyBg commented Dec 29, 2020 •

edited

Loading

devozerov Dec 30, 2020 •

edited

Loading

devozerov Dec 30, 2020

devozerov Dec 30, 2020

SeriyBg Dec 30, 2020

devozerov Dec 30, 2020

SeriyBg Dec 30, 2020

devozerov Dec 30, 2020

SeriyBg Dec 30, 2020

pivovarit Dec 31, 2020

SeriyBg Dec 31, 2020

pivovarit left a comment

pivovarit Dec 31, 2020

devozerov Jan 11, 2021

devozerov Jan 11, 2021

SeriyBg Jan 14, 2021

SeriyBg Jan 14, 2021

devozerov Jan 14, 2021

SeriyBg Jan 14, 2021

devozerov Jan 14, 2021

devozerov Jan 14, 2021

SeriyBg Jan 14, 2021

devozerov Jan 11, 2021

SeriyBg Jan 13, 2021

devozerov Jan 14, 2021

SeriyBg Jan 14, 2021

devozerov Jan 11, 2021

devozerov Jan 11, 2021

Types support #17

Types support #17

Conversation

SeriyBg commented Dec 29, 2020 • edited Loading

devozerov Dec 30, 2020 • edited Loading

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

pivovarit left a 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

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

SeriyBg commented Dec 29, 2020 •

edited

Loading

devozerov Dec 30, 2020 •

edited

Loading