ClaireLytt commented on PR #38669: URL: https://github.com/apache/shardingsphere/pull/38669#issuecomment-4534363334
> Merge Verdict: Not Mergeable > > Reviewed Scope > > PR latest head [bdee7e7](https://github.com/apache/shardingsphere/commit/bdee7e73f7c3d3fe0bcdf18d20fdee02369bf496) for [PR #38669](https://github.com/apache/shardingsphere/pull/38669). > > Hive SQL parser grammar/tests/release note only: > > parser/sql/engine/dialect/hive/src/main/antlr4/imports/hive/DDLStatement.g4 > > parser/sql/engine/dialect/hive/src/main/antlr4/imports/hive/HiveKeyword.g4 > > test/it/parser/src/main/resources/case/ddl/alter-table.xml > > test/it/parser/src/main/resources/sql/supported/ddl/alter-table.xml > > RELEASE-NOTES.md > > Official syntax evidence: [Apache Iceberg Hive DELETE ORPHAN-FILES docs](https://iceberg.apache.org/docs/latest/hive/#delete-orphan-files), which documents both no-argument and OLDER THAN (...) forms. > > Dialect-family scan: Hive only. The project SQL parsing standards list Hive with no branch dialect in CODE_OF_CONDUCT.md (line 120). > > Not Reviewed Scope > > Full parser IT suite was not run locally. > > Runtime HiveServer2/Iceberg execution was not reviewed. > > Need Expert Review > > Parser maintainer re-review is needed. No security/concurrency/performance expert review appears required. > > Positive Feedback > > The PR is small and focused. > > The two added happy-path parser cases match the Iceberg syntax for ALTER TABLE ... EXECUTE DELETE ORPHAN-FILES and ... OLDER THAN (...). > > Major Issues > > P1: OLDER is introduced as a global lexer keyword but not preserved as an identifier. > > Symptom: the PR adds OLDER in HiveKeyword.g4 and uses it in deleteOrphanFile, while alterTableCommonClause still resolves the table through tableName -> identifier. The identifier rule does not include OLDER. > > Risk: a previously valid Hive identifier such as table or column name older can stop parsing on existing paths, for example an adjacent ALTER TABLE older ... statement. This is an identifier-interpretation regression, and the PR only validates the new happy path. > > Recommended action: please either add OLDER to the appropriate Hive identifier keyword rule, likely identifierKeywordsUnambiguous, and add a regression case using older as an identifier on an existing Hive statement path, or provide official evidence that OLDER must become globally reserved in the targeted Hive/Iceberg syntax. > > Newly Introduced Issues > > The identifier compatibility issue above is newly introduced by this PR. I did not find additional unrelated code churn. > > Next-Step Suggestions > > Add a counterexample parser test for an existing Hive ALTER TABLE path where older is used as an identifier. > > Keep the two new DELETE ORPHAN-FILES cases. > > Re-run a scoped parser verification with dependencies, for example: > > ./mvnw -pl test/it/parser -am -DskipITs -Dspotless.skip=true -Dtest=org.apache.shardingsphere.test.it.sql.parser.hive.InternalHiveParserIT -Dsurefire.failIfNoSpecifiedTests=false test > > Then run the required gates from the project rules: ./mvnw spotless:apply -Pcheck -T1C and ./mvnw checkstyle:check -Pcheck -T1C. The Code of Conduct requires build/style/test checks in CODE_OF_CONDUCT.md (line 17), and consistency/readability in CODE_OF_CONDUCT.md (line 8). > > Multi-Round Comparison > > Previous review item: extra blank line in DDLStatement.g4 appears fixed. > > New blocker: identifier compatibility for the newly added OLDER token is still unvalidated. Done. Please review my new commit,thank you! -- 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]
