ClaireLytt commented on PR #38182: URL: https://github.com/apache/shardingsphere/pull/38182#issuecomment-3957492993
> Decision > > * Merge Verdict: Not Mergeable > * Reviewed Scope: Latest PR #38182 head ([5592d3c](https://github.com/apache/shardingsphere/commit/5592d3c4f64a595621952d608076c1313b6782fb)), all 6 changed files (RELEASE-NOTES.md, MySQL Keyword.g4/BaseRule.g4, MySQLStatementVisitor, and 2 parser IT SQL case files). > * Not Reviewed Scope: I did not deep-read full logs of all 148 CI jobs, and I did not rerun full parser IT locally. > * Need Expert Review: Recommended after fixes from a MySQL parser grammar maintainer. > > Positive feedback > > * The direction is good: grammar + visitor + IT SQL case + release note are all updated. > * CI evidence is strong on head commit: 148/148 check-runs are success. > > Major issues > > 1. Root-cause proof is still insufficient. > > * Current grammar already has functionCall -> regularFunction, and regularFunctionName allows identifier; visitor already extracts parameters in visitCompleteRegularFunction. > * Evidence: > > * BaseRule.g4 (/Users/zhangliang/IdeaProjects/shardingsphere/parser/sql/engine/dialect/mysql/src/main/antlr4/imports/mysql/BaseRule.g4:494) > * BaseRule.g4 (/Users/zhangliang/IdeaProjects/shardingsphere/parser/sql/engine/dialect/mysql/src/main/antlr4/imports/mysql/BaseRule.g4:735) > * MySQLStatementVisitor.java (/Users/zhangliang/IdeaProjects/shardingsphere/parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/MySQLStatementVisitor.java:1297) > * Risk: parser surface was expanded without showing that this specific path is required. > * Suggestion: please provide a minimal pre-fix failing case and expected-vs-actual AST evidence. > > Newly introduced issues > > 1. Blocker: compatibility regression risk. > > * LTRIM is added as a token, but not added into identifier’s unreservedWord. > * Evidence: > > * New token: Keyword.g4 (/Users/zhangliang/IdeaProjects/shardingsphere/parser/sql/engine/dialect/mysql/src/main/antlr4/imports/mysql/Keyword.g4:75) > * Identifier rule: BaseRule.g4 (/Users/zhangliang/IdeaProjects/shardingsphere/parser/sql/engine/dialect/mysql/src/main/antlr4/imports/mysql/BaseRule.g4:120) > * unreservedWord list: BaseRule.g4 (/Users/zhangliang/IdeaProjects/shardingsphere/parser/sql/engine/dialect/mysql/src/main/antlr4/imports/mysql/BaseRule.g4:128) > * Also, customKeyword containing LTRIM does not mitigate this (that rule is not referenced by grammar flow). > * Risk: ltrim may become invalid where it previously worked as an identifier. > > Next-step suggestions > > 1. Add LTRIM to unreservedWord to preserve identifier compatibility. > 2. Add regression SQL cases where ltrim is used as identifier (column/alias/object name), mapped to assertions. > 3. Provide pre-fix failure evidence proving why regularFunction path is insufficient. > 4. Re-run at least MySQL parser IT in test/it/parser and attach focused evidence. Thank you for your review opinion. I have updated the code which can run case successfully without changing visitor code. Please check it again! -- 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]
