ClaireLytt commented on PR #39131: URL: https://github.com/apache/shardingsphere/pull/39131#issuecomment-6396951504
### Result Review Result: Not Mergeable Feedback Mode: Change Request Blocking Issues: 5 The result-shape redesign is coherent across JDBC, Proxy headers, MySQL, PostgreSQL, and Firebird, and I confirmed no new production behavior defect. The change is still blocked because two added tests do not protect the repaired protocol paths and three newly introduced repository-standard violations remain. This is code-scope readiness only; CI was not reviewed. ### Blocking Issues #### 1. [P2] MySQL regression test does not verify the OK-packet column count - **Evidence:** `proxy/frontend/dialect/mysql/src/test/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/prepare/MySQLComStmtPrepareExecutorTest.java:412` adds an unhinted three-column test. At lines 443-451 it only counts `MySQLColumnDefinition41Packet` instances and never reads the `columnCount` written by `MySQLComStmtPrepareOKPacket` (`database/protocol/dialect/mysql/src/main/java/org/apache/shardingsphere/database/protocol/mysql/packet/command/query/binary/prepare/MySQLComStmtPrepareOKPacket.java:46`). - **Impact:** The previous `projections.size()` expression (base `MySQLComStmtPrepareExecutor.java:108`) would still satisfy this test, so a restored split between the OK count and the emitted definitions would pass while desynchronizing client statement metadata. - **Required Change:** Use a hinted query with an unexpanded wildcard and differing physical/logical counts, then assert both the OK packet's column count and the emitted column-definition count. #### 2. [P2] Firebird's new top-level shorthand path is untested - **Evidence:** `proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutor.java:266` adds the new branch only when the `FROM` is not a `SubqueryTableSegment`. Both added hinted tests (`FirebirdPrepareStatementCommandExecutorTest.java:487` and `:502`) use `FROM (SELECT ...) s`, so they bypass it. JaCoCo after the 31 passing tests shows the new branch and `isAliasOf` unexecuted. - **Impact:** Incorrect wildcard expansion or table-alias filtering on ordinary hinted SELECTs would remain undetected. - **Required Change:** Exercise a hinted ordinary-table wildcard and a qualified wildcard in a JOIN through `execute()`, asserting visible count, order, and ownership. A live database is not required. #### 3. [P3, explicit standard] New Firebird null guard is unsupported - **Evidence:** `FirebirdPrepareStatementCommandExecutor.java:336` checks `getTablesContext()` for null. Its only caller is restricted to `SelectStatementContext`, and `SelectStatementBaseContext` always constructs a non-null `TablesContext` (`SelectStatementBaseContext.java:122`). This violates the repository rule against revalidating upstream non-null guarantees (`defensive-code.md:38`). - **Impact:** Standards-only issue; no runtime defect is alleged. - **Required Change:** Remove only the newly added null branch. #### 4. [P3, explicit standard] Added line comments use a prohibited form - **Evidence:** `proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/response/header/query/QueryHeaderBuilderEngineTest.java:47` adds two `//` comments. `CODE_OF_CONDUCT.md:71` permits only Javadoc, `TODO`, and `FIXME`. - **Impact:** Standards-only issue; Checkstyle and Spotless do not enforce this manual rule. - **Required Change:** Preserve the explanation as Javadoc. #### 5. [P3, explicit standard] MySQL private-helper declarations are out of call order - **Evidence:** `MySQLComStmtPrepareExecutor.java:109` now calls `createProjectionColumnDefinition41Packets` before `createParameterColumnDefinition41Packets` at line 114, but the declarations remain in the opposite order at lines 124 and 169. This violates `CODE_OF_CONDUCT.md:48`. - **Impact:** Standards-only issue; the runtime packet order is correct. - **Required Change:** Align the private-helper declarations with their call order without changing behavior. ### Coverage - **Candidate:** Public PR #39131, head `87ae3639538d29217e44884cf86019edb3441f33`, base/merge-base `50cd58771c484f5f6b281a349aff9c3f7c27ffbb`. The GitHub file list and local triple-dot list agree on all 18 authoritative files. - **Requirements and accepted behavior:** Issue #28841 and the public maintainer discussion establish the hinted unexpanded-shorthand problem and the shared visible-shape boundary. No new database, topology, or SPI commitment was required. - **Behavior clusters:** projection state and visible mapping; JDBC ResultSet and metadata; Proxy headers; MySQL prepare; PostgreSQL describe; Firebird prepare; regression tests and release note. - **Discovery and adjudication:** Two blind independent assessments each covered all 18 files and 55 changed blocks, with all three discovery lenses and convergence. I rechecked every claim's full path and counter-evidence. Overlapping findings were merged by fix boundary; prior claims about physical-metadata internal-column leakage and the MySQL/PostgreSQL consumer split were withdrawn against current code. No unresolved candidate or `Review Incomplete` gap remains. - **Confirmed non-blocking:** mapping reuse/caching, simplification opportunities, E2E cases that assert only routing, and pre-existing fallback limitations. - **Style verification (local, not CI):** Checkstyle over the six owning modules with exact includes exited `0`, with 0 violations and reports proving all 16 changed Java files were selected. Spotless with exact file filters exited `0` and reports all six modules clean. `RELEASE-NOTES.md` and the E2E XML were `Not Applicable` to Java style checks. - **Runtime evidence:** The JDBC shorthand IT passed 8/8, and the affected unit suites passed, including 31 Firebird tests. These passing tests do not cover the two blocking gaps above. - **CI scope:** GitHub Actions and checks were not queried. This result reflects code-scope readiness only; local style verification was completed separately. - **Tooling note:** The repository ledger validator cannot run on this Windows host because it calls Unix-only `os.getuid()`. Ledger review was completed read-only from the recorded 18-file coverage, confirmed findings, and zero pending/blocked files; no successful `validate-session` claim is made. -- 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]
