Fix to handle version-specific integration tests #1037
Conversation
jnmt
force-pushed
the
fix-version-specific-integration-tests
branch
from
6956033
to
0782ebe
Compare
.github/workflows/ci.yaml
Outdated
| @@ -240,7 +240,7 @@ jobs: | |||
| - name: Setup and execute Gradle 'integrationTestJdbc' task | |||
| uses: gradle/gradle-build-action@v2 | |||
| with: | |||
| arguments: integrationTestJdbc -Dscalardb.jdbc.url=jdbc:postgresql://localhost:5432/ -Dscalardb.jdbc.username=postgres -Dscalardb.jdbc.password=postgres | |||
| arguments: integrationTestJdbc -Dscalardb.jdbc.url=jdbc:postgresql://localhost:5432/ -Dscalardb.jdbc.username=postgres -Dscalardb.jdbc.password=postgres -Dscalardb.jdbc.database.version.major=14 | |||
There was a problem hiding this comment.
When you don't specify versions, it will work with the version = max integer. It can work without specifying the version in most cases if there is no critical change around the version in the database. But, specifying version is now recommended for PostgreSQL and Oracle just in case (and might also be for MySQL and SQL Server in the future).
There was a problem hiding this comment.
Instead of having to set the database's major version as an external parameter, it seems we can retrieve this data using the JDBC driver with
dataSource.getConnection().getMetaData().getDatabaseMajorVersion()
cf. the Javadoc
We could then have the integration test directly retrieve this data. What do you think ?
There was a problem hiding this comment.
@Torch3333 Thank you! I didn't know it. Let me check how it works.
There was a problem hiding this comment.
@Torch3333 I could get the expected versions using the JDBC driver except for MariaDB. Although MariaDB returns a bit confusing name and versions due to (maybe) the use of MySQL driver, currently, we don't care about versions for compatibility tests with MySQL/MariaDB. So, I will go with your suggestion.
FYI: the returned versions from each database are as follows.
# Name: ProductName, Version: ProductVersion, Major: MajorVersion, Minor: MinorVersion
Name: MySQL, Version: 8.1.0, Major: 8, Minor: 1
Name: MySQL, Version: 5.5.5-10.11.5-MariaDB-1:10.11.5+maria~ubu2204, Major: 5, Minor: 5
Name: PostgreSQL, Version: 14.9 Major: 14 Minor: 9
Name: Oracle, Version: Oracle Database 18c Express Edition Release 18.0.0.0.0 - Production Version 18.4.0.0.0, Major: 18, Minor: 0
Name: Microsoft SQL Server, Version: 16.00.4065, Major: 16 Minor: 0There was a problem hiding this comment.
@Torch3333 Fixed in 5feddc5. PTAL!
There was a problem hiding this comment.
Thank you.
Although MariaDB returns a bit confusing name and versions due to (maybe) the use of MySQL driver,
Yes, I guess so.
| private boolean isMariaDB() { | ||
| try (BasicDataSource dataSource = JdbcUtils.initDataSourceForAdmin(config, rdbEngine); | ||
| Connection connection = dataSource.getConnection()) { | ||
| String version = connection.getMetaData().getDatabaseProductVersion(); | ||
| return version.contains("MariaDB"); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException("Get database major version failed"); | ||
| } | ||
| } | ||
|
|
||
| private int getMajorVersion() { | ||
| try (BasicDataSource dataSource = JdbcUtils.initDataSourceForAdmin(config, rdbEngine); | ||
| Connection connection = dataSource.getConnection()) { | ||
| return connection.getMetaData().getDatabaseMajorVersion(); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException("Get database major version failed"); | ||
| } | ||
| } |
| String version = connection.getMetaData().getDatabaseProductVersion(); | ||
| return version.contains("MariaDB"); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException("Get database major version failed"); |
There was a problem hiding this comment.
| throw new RuntimeException("Get database major version failed"); | |
| throw new RuntimeException("Get database product version failed"); |
Sorry, I missed that.
There was a problem hiding this comment.
Good catch! Thank you! Fixed in 7cb850a.
This PR introduces version-specific integration tests for the table importing feature to fix issues found in the supported storage compatibility tests. Although I also checked the new supported version (Oracle 23, MySQL 8.1, PostgreSQL 15, SQL Server 2022), I don't think there are updates related to supported data types for importing. You can see the latest results of compatibility tests here. Note that the failure in Cassandra's tests is not related to this PR.
By the way, I apparently found a bug that might happen when importing a table with minor data types in SQL Server, regardless of the versions. So, I will fix it in another PR.