terrymanu commented on PR #39509:
URL: https://github.com/apache/shardingsphere/pull/39509#issuecomment-5331249164
### Result
**Review Result: Mergeable**
The PR correctly completes the two remaining dialect loaders in issue #29816
(H2 and SQLServer) with the same view-classification pattern already merged for
MySQL, PostgreSQL, Oracle, and OpenGauss, and fixes the MySQL `SHOW FULL
TABLES` type column to match native MySQL output. No candidate passed the
finding proof gate as a blocking issue.
### Evidence
- **H2 view collection**
(`database/connector/dialect/h2/src/main/java/org/apache/shardingsphere/database/connector/h2/metadata/data/loader/H2MetaDataLoader.java:69-105`):
the new `VIEW_META_DATA_SQL` filters `INFORMATION_SCHEMA.TABLES` by
`TABLE_CATALOG`/`TABLE_SCHEMA`/`TABLE_TYPE='VIEW'` with `UPPER(TABLE_NAME) IN
(...)`, exactly matching the filter shape of the four pre-existing queries in
the same class; the empty-`IN` case is guarded by the
`columnMetaDataMap.isEmpty()` check. `sys`-style classification
(`viewNames.contains(key) ? VIEW : TABLE`) is identical to
`MySQLMetaDataLoader.java:81`, `OracleMetaDataLoader.java:109`,
`PostgreSQLMetaDataLoader.java:271`, and `OpenGaussMetaDataLoader.java:225`.
- **SQLServer view collection**
(`database/connector/dialect/sqlserver/src/main/java/org/apache/shardingsphere/database/connector/sql92/sqlserver/metadata/data/loader/SQLServerMetaDataLoader.java:65-112`):
`sys.objects WHERE type = 'V'` is the documented catalog value for views; name
matching without `UPPER` is consistent with the existing `obj.name IN (%s)`
queries; called only inside the non-empty `columnMetaDataMap` branch. The
existing column query already reads view columns from `sys.objects`, so views
were previously loaded but misclassified as `TABLE`.
- **MySQL output fix**
(`proxy/backend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/backend/mysql/handler/admin/executor/show/MySQLShowTablesExecutor.java:112-115`):
`TableType` has exactly two values (`TABLE`, `VIEW`), so the `VIEW`/`BASE
TABLE` mapping is exhaustive; the previous code printed the enum name `TABLE`,
which native MySQL never emits. A repository-wide search found no test, E2E
case, or consumer asserting the old `TABLE` output; the only `BASE TABLE`
references are parser input cases and an E2E pipeline query against real MySQL,
both unaffected.
- **Local verification** (read-only; PR head exported via `git archive`,
built with `-am` so upstream `TableMetaData`/`LocalDataQueryResultRow` changes
from Aug 17–18 were compiled from source rather than stale Aug 16 local
artifacts): `H2MetaDataLoaderTest` (3 tests), `SQLServerMetaDataLoaderTest` (6
tests), and `MySQLShowTablesExecutorTest` (7 tests) all passed, exit code 0.
- **Test stub validity**: the tests stub exact SQL strings whose `IN`-clause
order depends on `HashMap` key iteration; verified via a standalone JVM check
that `{"tbl", "tbl_view"}` iterates as `[tbl_view, tbl]`, matching both new
stubs (column query uses the input list order, view query uses the map key
order).
### Coverage
- Reviewed head: `664349e27b5151ad03f789ad1af3167685d078d1` (single commit);
merge-base `6a4dc56a185`; GitHub changed-file list matched the local triple-dot
list exactly (6/6 files: 3 production, 3 tests).
- Requirements: PR description and issue #29816 task list — the PR covers
exactly the two unchecked loaders (H2, SQLServer) plus the `SHOW FULL TABLES`
alignment described in the issue body.
- Behavior clusters: H2 view-type collection; SQLServer view-type
collection; MySQL `SHOW FULL TABLES` type rendering. Every changed file maps to
one cluster; no churn-only files.
- Discovery lenses completed: root cause and behavior, blast radius and
contracts (no public API/SPI change; output contract change is the intended
fix; one extra catalog query per non-empty load, mirroring the four merged
loaders), tests/runtime/operations. Final convergence pass found no new
independent candidate.
- Non-blocking observation: the new H2/SQLServer SQL is verified by
mock-based unit tests and dialect documentation patterns only; no live-database
integration test for these loaders exists in the repository (pre-existing gap,
consistent with in-class precedent).
- CI scope: Code Correctness Review only — this result is code-scope
readiness; GitHub Actions were not queried or reviewed. No PR reviews or
comments existed at review time.
--
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]
