terrymanu commented on PR #38879: URL: https://github.com/apache/shardingsphere/pull/38879#issuecomment-4757940907
### Summary - **Merge Decision: Mergeable** - **Reason:** The latest version fixes the JDBC stream SQLXML retrieval path directly and includes focused regression coverage for the new branch. ### Evidence - `ShardingSphereResultSet#getSQLXML` already delegates to `mergeResultSet.getValue(columnIndex, SQLXML.class)`, and this PR now makes `JDBCStreamQueryResult#getValue(..., SQLXML.class)` call `ResultSet#getSQLXML(columnIndex)` directly at `infra/executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/execute/result/query/impl/driver/jdbc/type/stream/JDBCStreamQueryResult.java:118`. - The focused regression test `assertGetValueBySQLXML` stubs `ResultSet#getSQLXML(1)` and verifies `JDBCStreamQueryResult#getValue(1, SQLXML.class)` returns the same `SQLXML` instance at `infra/executor/src/test/java/org/apache/shardingsphere/infra/executor/sql/execute/result/query/impl/driver/jdbc/type/stream/JDBCStreamQueryResultTest.java:167`. - The added tests also cover adjacent existing stream-result branches for `LocalTime`, default `Object`, and `getJDBCResultSet()` contained/not-contained behavior, bringing `JDBCStreamQueryResult` to 100% class, line, and branch coverage in the scoped Jacoco report. - Scope is narrow and owned by `infra/executor`: one production branch plus direct tests, with no public API, config, dependency, distribution, protocol, or documentation changes. - No shared-layer target-specific leakage, new lifecycle state, high-frequency map/cache change, or unrelated cleanup was found in the latest two-file diff. ### Review Details - **Reviewed Scope:** PR #38879 latest head `f70eee32172ee5cc5fa35a80692fbbce88899606`, base `master` at `f96a65898f4525916d9074ea789207dd277f4a6d`, merge-base `f96a65898f4525916d9074ea789207dd277f4a6d`; local triple-dot file list matched GitHub `/pulls/38879/files`. Reviewed `infra/executor/src/main/java/org/apache/shardingsphere/infra/executor/sql/execute/result/query/impl/driver/jdbc/type/stream/JDBCStreamQueryResult.java`, `infra/executor/src/test/java/org/apache/shardingsphere/infra/executor/sql/execute/result/query/impl/driver/jdbc/type/stream/JDBCStreamQueryResultTest.java`, and the relevant caller/merge paths in `ShardingSphereResultSet`, `TransparentMergedResult`, `StreamMergedResult`, and `MemoryMergedResult`. - **Not Reviewed Scope:** GitHub Actions/check-run status, full `clean install`, and live JDBC/Proxy SQLXML smoke against a real database driver. - **Verification:** `./mvnw -pl infra/executor -DskipITs -Dspotless.skip=true -Dtest=JDBCStreamQueryResultTest -DfailIfNoTests=true -Dsurefire.failIfNoSpecifiedTests=false test` exited `0` with `Tests run: 35`; scoped Jacoco report exited `0` and showed `JDBCStreamQueryResult` CLASS `100%`, LINE `100%`, BRANCH `100%`; `./mvnw spotless:check -Pcheck -T1C` exited `0`; `./mvnw checkstyle:check -Pcheck -T1C` exited `0`. The module-scoped test did not use `-am` because the changed production class and direct test are both in `infra/executor`, so reactor dependency freshness was not needed for this focused path. - **Release Note / User Docs:** Not required; this is a narrow internal JDBC stream-result delegation fix that restores the existing JDBC `getSQLXML` retrieval path without new configuration, migration steps, or user-facing documentation changes. -- 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]
