[GitHub] [shardingsphere] linghengqian commented on a diff in pull request #19964: Update H2DataBase to 2.x and drop support for H2Database 1.x

[GitBox <gi...@apache.org>]

linghengqian commented on code in PR #19964:
URL: https://github.com/apache/shardingsphere/pull/19964#discussion_r939865125


##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/test/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatementTest.java:
##########
@@ -58,7 +58,7 @@ public void assertGetGeneratedKeys() throws SQLException {
             generatedKeysResultSet = statement.getGeneratedKeys();
             assertTrue(generatedKeysResultSet.next());
             assertThat(generatedKeysResultSet.getLong(1), is(6L));
-            assertFalse(statement.execute(String.format(sql, 1, 1, "init"), new String[]{"no"}));
+            assertFalse(statement.execute(String.format(sql, 1, 1, "init"), new String[]{"status"}));

Review Comment:
   The field name was wrong, fixed.



##########
shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-suite/src/test/java/org/apache/shardingsphere/test/integration/engine/ddl/BaseDDLIT.java:
##########
@@ -139,7 +139,8 @@ private List<DataSetColumn> getActualColumns(final Connection connection, final
             while (resultSet.next()) {
                 DataSetColumn each = new DataSetColumn();
                 each.setName(resultSet.getString("COLUMN_NAME"));
-                each.setType(resultSet.getString("TYPE_NAME").toLowerCase());
+                String typeName = resultSet.getString("TYPE_NAME");
+                each.setType("CHARACTER VARYING".equals(typeName) ? "varchar" : typeName.toLowerCase());

Review Comment:
   - I think the behavior in the context of `BaseDDLIT#assertTableMetaData()` is very strange. 
   
   - `DataSetMetaData expected = getDataSet().findMetaData(tableName);` for this function, get `shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-suite/src/test/resources/cases/ddl/dataset/db/create_table.xml` similar file with `DataSetLoader#getFile()`, get the expected `TYPE_NAME`. 
   
   - This is compared to the `TYPE_NAME` that the `BaseDDLIT#getActualColumns` method gets from `getActualDataSourceMap().get(each.getDataSourceName()).getConnection()`, the expected `VARCHAR` is always `CHARACTER VARYING`. 
   
   - A minor problem is that `java.sql.Types` does not have `CHARACTER VARYING`, I don't know what's going on.
   
   - I don't Know if this is a legacy issue, after all so many IT modules only need to change this file.



##########
shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/metadata/database/schema/loader/dialect/H2SchemaMetaDataLoader.java:
##########
@@ -104,11 +105,10 @@ private Map<String, Collection<ColumnMetaData>> loadColumnMetaDataMap(final Conn
     private ColumnMetaData loadColumnMetaData(final Map<String, Integer> dataTypeMap, final ResultSet resultSet, final Collection<String> primaryKeys,
                                               final Map<String, Boolean> tableGenerated) throws SQLException {
         String columnName = resultSet.getString("COLUMN_NAME");
-        String typeName = resultSet.getString("TYPE_NAME");
+        String dataType = resultSet.getString("DATA_TYPE");
         boolean primaryKey = primaryKeys.contains(columnName);
         boolean generated = tableGenerated.getOrDefault(columnName, Boolean.FALSE);
-        // H2 database case sensitive is always true
-        return new ColumnMetaData(columnName, dataTypeMap.get(typeName), primaryKey, generated, true, true);

Review Comment:
   This is definitely not reasonable behavior, so the unit tests related to this comment have been modified.



##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/test/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSpherePreparedStatementTest.java:
##########
@@ -430,7 +430,7 @@ public void assertAddOnDuplicateKey() throws SQLException {
             preparedStatement.setString(8, status);
             preparedStatement.setString(9, updatedStatus);
             int result = preparedStatement.executeUpdate();
-            assertThat(result, is(2));
+            assertThat(result, is(4));

Review Comment:
   I'm not sure why `H2Database 1.x` is `2`. At least `4` is reasonable according to the processing logic of `H2Database 2.x`, I don't know what I overlooked.



##########
shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-suite/src/test/java/org/apache/shardingsphere/test/integration/engine/ddl/BaseDDLIT.java:
##########
@@ -139,7 +139,8 @@ private List<DataSetColumn> getActualColumns(final Connection connection, final
             while (resultSet.next()) {
                 DataSetColumn each = new DataSetColumn();
                 each.setName(resultSet.getString("COLUMN_NAME"));
-                each.setType(resultSet.getString("TYPE_NAME").toLowerCase());
+                String typeName = resultSet.getString("TYPE_NAME");
+                each.setType("CHARACTER VARYING".equals(typeName) ? "varchar" : typeName.toLowerCase());

Review Comment:
   Since https://www.h2database.com/html/datatypes.html does not exist for VARCHAR, I realize that this class may have to be changed. Refer to https://github.com/h2database/h2database/issues/3359#issuecomment-1009780245.



-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org