Fix incorrect return value of SQLFederationResultSetMetaData#getColumnClassName#39217
Fix incorrect return value of SQLFederationResultSetMetaData#getColumnClassName#39217PRAHLAD09-dev wants to merge 4 commits into
Conversation
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The patch fixes the enum-class symptom for VARCHAR, but still reports classes incompatible with actual SQL Federation getObject values for Java-backed and dialect-converted columns. The linked issue’s required regression coverage is also incomplete.
Issues
P1 — Keep getColumnClassName consistent with getObject
Problem: getColumnClassName derives the class solely from getColumnType. This conflicts with the JDBC contract, which requires a class compatible with values produced by ResultSet#getObject:
- A
JavaTypebacked byBigIntegeris forced to JDBCBIGINTat lines 154-160, then reported asLong, while SQL Federation preserves theBigIntegervalue throughgetObject. MySQL unsignedBIGINTis explicitly modeled asBigInteger. - The MySQL converter converts Boolean values to
Integervalues (0/1) but converts their JDBC type toVARCHAR, so this patch reportsString.
Neither BigInteger/Long nor Integer/String has the assignability relationship permitted by the Java SE 8 JDBC contract.
Impact: Frameworks selecting handlers from metadata can still choose an incompatible handler for valid SQL Federation results, reproducing the same failure category as #39121.
Required Change: Please resolve the reported class from the actual SQL Federation value/converter contract and cover at least the Java-backed BigInteger and MySQL Boolean/ANY paths, ensuring the reported class is compatible with getObject.
P2 — Complete the requested regression coverage
Problem: assertGetColumnClassName checks only one mocked VARCHAR case. The latest-head full-module JaCoCo report shows 1 covered and 11 missed branches in the new classifier. It does not reproduce the reported BIGINT path or exercise numeric, temporal, binary, fallback, Java-backed, or dialect-converted behavior.
The linked issue’s accepted scope explicitly requests representative numeric, character, temporal, binary, and fallback coverage plus a ShardingSphere-JDBC SQL Federation metadata sentinel.
Impact: The current test allowed the confirmed class/value mismatches above and does not prove the root-cause path through the JDBC-facing result set.
Required Change: Please add parameterized focused coverage for the requested type groups and one ShardingSphere-JDBC SQL Federation test asserting both getColumnClassName and the corresponding getObject class/value.
Review Details
- Review Focus: Code Correctness Review — CI not reviewed by request.
- Reviewed Scope: Both changed files in
kernel/sql-federation/core; latest head6370fd1e713e2acdca835faef9e081bcf5c10965; basemasteratda096439aa116e5f15cebee32a01671ccb3e662c; local merge-base80e6541874d47b5814e3d1400c925d6ff3afa0ed. The local triple-dot file list matched GitHub/pulls/39217/files. - Not Reviewed Scope: GitHub Actions, check-runs, and workflow logs; database-backed JDBC/E2E execution; unrelated modules.
- Verification: Latest-head focused test: 37 tests passed, exit 0. Full
kernel/sql-federation/coretests with JaCoCo: 259 tests passed, exit 0; new classifier branches: 1 covered, 11 missed. Module Spotless and Checkstyle checks both passed, exit 0. Required PR, issue, file, comment, and review evidence was accessible; no evidence-access gap affected this result. - Release Note / User Docs: Not required; this is an internal bug fix with no configuration, API/SPI signature, migration, or operational change.
|
Thanks for the review. I revisited the implementation based on your feedback and traced how SQLFederationResultSet#getObject() actually obtains its value. getObject() eventually goes through getValue(), which applies DialectSQLFederationColumnTypeConverter#convertColumnValue(). So getColumnClassName() should describe the Java type after that dialect-specific conversion, rather than relying only on the JDBC type returned by getColumnType(). I updated the implementation accordingly: Added a dialect hook for reporting the Java class produced by column value conversion. I also added regression coverage for the BigInteger case, dialect-converted class behavior, the JDBC type mappings, and MySQL BOOLEAN -> Integer. I verified the changes with the SQL federation core and MySQL tests, including the reactor build, and they pass successfully. Updated in commit 4c05f42. |
Description
Fixes #39121.
Changes proposed in this pull request
SQLFederationResultSetMetaData#getColumnClassName.SqlTypeNameenum class.