terrymanu commented on PR #39014:
URL: https://github.com/apache/shardingsphere/pull/39014#issuecomment-4886675445
### Summary
**Review Result: Mergeable**
Reason: The latest PR head is mergeable for code correctness scope only. The
change fixes the metadata URL fallback trigger point for drivers that throw
plain `SQLException` from `DatabaseMetaData#getURL()`, while preserving the
original exception when no dialect URL fetcher applies. CI not reviewed by
request.
### Evidence
- The production change is limited to
`database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/type/DatabaseTypeFactory.java:64`,
where `get(DatabaseMetaData)` now catches `SQLException` and still calls
`findByDialectJdbcUrlFetcher(...).orElseThrow(() -> ex)`. This preserves the
previous passthrough behavior when no dialect fetcher matches.
- The Hive fallback path remains owned by the existing SPI implementation:
`database/connector/dialect/hive/src/main/java/org/apache/shardingsphere/database/connector/hive/jdbcurl/HiveJdbcUrlFetcher.java:32`
fetches the URL from `HiveConnection#getConnectedUrl()`, and
`HiveDatabaseType` recognizes `jdbc:hive2:` URLs.
- The new tests cover both important boundaries:
-
`database/connector/core/src/test/java/org/apache/shardingsphere/database/connector/core/type/DatabaseTypeFactoryTest.java:95`
verifies plain `SQLException` can still resolve through a matching
`DialectJdbcUrlFetcher`.
-
`database/connector/core/src/test/java/org/apache/shardingsphere/database/connector/core/type/DatabaseTypeFactoryTest.java:126`
verifies plain `SQLException` is preserved when no fetcher matches.
- Public upstream evidence supports the compatibility target: Apache Hive
issue HIVE-15540 records `java.sql.SQLException: Method not supported` from
`HiveDatabaseMetaData.getURL`, and Hive 2.1.1 / 2.2.0 source uses plain
`SQLException` for that method. This matches the root-cause path fixed by the
PR.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39014 latest head
`6143a10c1fb4b32a910b97ca6de985b3091a0e22`, base `master` at
`1f47e5ccf21b775c3aa18d57172882754a5a4955`, merge-base
`1f47e5ccf21b775c3aa18d57172882754a5a4955`. Reviewed files:
`DatabaseTypeFactory.java` and `DatabaseTypeFactoryTest.java`. Local triple-dot
file list matched GitHub `/pulls/39014/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, full repository build,
runtime HiveServer2 smoke/E2E, and unrelated local working-tree changes outside
the PR.
- Verification: Ran `./mvnw -pl database/connector/core -DskipITs
-Dspotless.skip=true
-Dtest=org.apache.shardingsphere.database.connector.core.type.DatabaseTypeFactoryTest
-Dsurefire.failIfNoSpecifiedTests=false test`, exit code 0; `Tests run: 11,
Failures: 0, Errors: 0, Skipped: 0`.
- Release Note / User Docs: Not required. This is an internal compatibility
fallback fix with no new user-facing configuration, API, or migration behavior.
--
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]
