yx9o commented on PR #38989: URL: https://github.com/apache/shardingsphere/pull/38989#issuecomment-4882387363
### Summary **Review Result: Mergeable** **Reason:** Code-scope review only. CI not reviewed by request. The latest patch addresses the linked PostgreSQL `VALUES` derived-table alias-column binding path: it preserves the table alias separately from the alias column list, creates projections for `VALUES` subqueries, and lets the subquery table binder expose those alias columns as temporary-table projections for downstream owner-column binding. ### Evidence - The linked issue `#38643` fails because PostgreSQL `AS t(pk, col_a, col_a_type)` was flattened into one alias string and the `VALUES` subquery had no projections. The PR fixes both propagation points: `PostgreSQLStatementVisitor` now creates projections when `simpleSelect` is a `valuesClause` and keeps `AliasSegment.identifier` as `t` while storing `pk`, `col_a`, and `col_a_type` in `columnAliases` (`parser/sql/engine/dialect/postgresql/src/main/java/org/apache/shardingsphere/sql/parser/engine/postgresql/visitor/statement/PostgreSQLStatementVisitor.java:1076-1086`, `:1110-1118`, `:1437-1444`). - The binder now consumes alias-column lists from `AliasSegment` when the `SubqueryTableSegment` itself has no columns, validates the alias-column count against the subquery projections, and replaces the temporary table projections with the alias column labels (`infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/from/type/SubqueryTableSegmentBinder.java:85-94`, `:100-113`, `:134-147`). That is the path needed for later `t.col_a_type` / `t.col_a` owner lookup. - Regression coverage matches the repaired path at the parser and binder layers: PostgreSQL visitor tests cover both `SELECT ... FROM (VALUES ...) AS t(...)` and `UPDATE ... FROM (VALUES ...) AS t(...)`, including alias-column extraction and generated `VALUES` projections (`parser/sql/engine/dialect/postgresql/src/test/java/org/apache/shardingsphere/sql/parser/engine/postgresql/visitor/statement/PostgreSQLStatementVisitorTest.java:63-90`). Binder tests cover alias-column binding, temporary-table projection labels, and mismatched column-count validation (`infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/from/type/SubqueryTableSegmentBinderTest.java:178-225`). ### Review Details **Review Focus:** Code Correctness Review. CI not reviewed by request. **Reviewed Scope:** PR #38989 latest head `9ee882634d89258fa0fcbe93534c5b942c8a4f26`, base `master` at `a2c8572d344b3ccd7846e68ded163250d206ed63`. GitHub `/pulls/38989/files` returned 7 files and matched the reviewed raw head sources: PostgreSQL visitor/test, `AliasSegment`, `SubqueryTableSegmentBinder`/test, and parser baseline XML files. **Not Reviewed Scope:** GitHub Actions/check-runs and full Maven execution were not reviewed. I also did not treat broader PostgreSQL simple-table alias-column-list behavior as part of this linked `VALUES` subquery fix. **Verification:** Reviewed public PR metadata, commits, file list, issue #38643, public comments/reviews, REST file patches, and raw head sources. Public comments and reviews were empty. `patch-diff.githubusercontent.com` timed out during diff download, but `/pulls/38989/files` and raw head sources were accessible, so this did not affect the code-scope result. No local Maven command was run. **Release Note / User Docs:** Not required for this internal PostgreSQL parser/binder bug fix; no configuration, migration, or user-facing option changed. -- 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]
