Re: [PR] Support Hive parser and binder for insert select columns and transform expressions [shardingsphere]

["strongduanmu (via GitHub)" <gi...@apache.org>]

strongduanmu commented on PR #39269:
URL: https://github.com/apache/shardingsphere/pull/39269#issuecomment-5116698235

   • ### Result
     **Review Result: Mergeable**
   
     This is a code-correctness review for PR `apache/shardingsphere#39269` at head `4762f17a68cdd9d5273410fda599d8687d3bb652`.
   
     No blocking issue was found in the changed Hive parser, SQL statement model, Binder logic, or related tests. Local CI checks passed. GitHub Actions were still pending at the time of review, so the mergeability conclusion is based on
     local verification and code review evidence.
   
     ### Evidence
     Reviewed scope:
     - Hive parser changes for `INSERT ... SELECT`, `WITH ... INSERT`, `TRANSFORM`, `MAP`, `REDUCE`, grouping extensions, and aggregation windows.
     - SQL statement model changes for `SelectStatement`, `InsertStatement`, and `SimpleTableSegment`.
     - Binder changes for projections, aggregation distinct projections, simple table segments, insert statements, and select statements.
     - Corresponding unit tests and parser IT case updates.
   
     Local verification passed:
     - `./mvnw spotless:check -Pcheck -T1C ...`
     - `./mvnw checkstyle:check -Pcheck -T1C ...`
     - Parser scoped tests:
       - `SelectStatementTest`
       - `HiveStatementVisitorTest`
     - Binder scoped tests:
       - `AggregationDistinctProjectionSegmentBinderTest`
       - `SimpleTableSegmentBinderTest`
       - `InsertStatementBinderTest`
       - `SelectStatementBinderTest`
   
     ### Review Notes
     The parser changes preserve the existing Hive visitor flow while adding missing AST metadata for Hive-specific query forms. The Binder changes copy the newly modeled metadata instead of changing downstream rewrite behavior.
   
     The `overwrite` field is currently copied by the Binder and covered by tests. I did not find a current production consumer that depends on this field yet, so this is treated as model completeness rather than a behavioral risk.
   
     The grouping extension handling uses the existing `GroupBySegment` representation and marks grouping extension presence explicitly. This matches the current model shape and is covered by visitor tests.
   
     ### Coverage
     Files reviewed include parser grammar, Hive visitor, core statement model classes, Binder classes, release note, and related tests. The review did not identify regressions in the changed code paths.
   
     Conclusion: the PR is acceptable from local CI and code-correctness review perspective. Continue to monitor GitHub Actions; if a remote job fails, handle that job’s log directly.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org