jnturton commented on a change in pull request #2432:
URL: https://github.com/apache/drill/pull/2432#discussion_r795532467
##########
File path:
contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcWriterWithH2.java
##########
@@ -142,14 +142,14 @@ public void testBasicCTASWithDataTypes() throws Exception
{
DirectRowSet results = queryBuilder().sql(testQuery).rowSet();
TupleMetadata expectedSchema = new SchemaBuilder()
- .addNullable("int_field", MinorType.INT, 10)
- .addNullable("bigint_field", MinorType.BIGINT, 19)
- .addNullable("float4_field", MinorType.VARDECIMAL, 38, 37)
- .addNullable("float8_field", MinorType.VARDECIMAL, 38, 37)
+ .addNullable("int_field", MinorType.INT, 32)
+ .addNullable("bigint_field", MinorType.BIGINT, 38)
+ .addNullable("float4_field", MinorType.FLOAT4, 38)
+ .addNullable("float8_field", MinorType.FLOAT8, 38)
Review comment:
@vdiravka no - this change is not specific to H2. I noticed while
debugging H2 test failures that the new JDBC writer code maps Drill's FLOAT4
AND FLOAT8 to the JDBC data type NUMERIC (a vardecimal type). Apparently the
idea was to try to avoid inconsistent float support across databases, but I
don't think this is a good idea. Vardecimal columns have a fixed precision and
scale, making them fixed point rather floating point, and introducing the
potential for data conversion problems.
For example, in `TestJdbcWriterWithH2#testBasicCTASWithDataTypes` if you
just change a test value by one decimal place the unit test fails:
```
--- "CAST(3.0 AS FLOAT) AS float4_field," +
+++ "CAST(30.0 AS FLOAT) AS float4_field," +
```
because it has tried to convert a FLOAT4 to a DECIMAL(38,37) which can only
represents numbers smaller than 10. Since I found this while trying to upgrade
H2, I decided to try to fix it too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
