terrymanu commented on PR #38669: URL: https://github.com/apache/shardingsphere/pull/38669#issuecomment-4560564374
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** Latest PR head `c5ff78ed00da12a9d16d009ed1a4b47b01d95ba8` for Hive SQL parser support in `parser/sql/engine/dialect/hive/src/main/antlr4/imports/hive/DDLStatement.g4:84`, `parser/sql/engine/dialect/hive/src/main/antlr4/imports/hive/DDLStatement.g4:539`, `parser/sql/engine/dialect/hive/src/main/antlr4/imports/hive/HiveKeyword.g4:3577`, `parser/sql/engine/dialect/hive/src/main/antlr4/imports/hive/BaseRule.g4:337`, parser IT fixtures in `test/it/parser/src/main/resources/sql/supported/ddl/alter-table.xml:377`, `test/it/parser/src/main/resources/case/ddl/alter-table.xml:2524`, and `RELEASE-NOTES.md:28`. Official syntax evidence checked: [Apache Iceberg Hive DELETE ORPHAN-FILES docs](https://iceberg.apache.org/docs/latest/hive/#delete-orphan-files) and [Apache Hive keyword docs](https://hive.apache.org/docs/latest/language/languagemanual-ddl/#keywords-non-reserved-keywords-and-reserved-keywords). Dialect-family scan: Hive only; project parser maintenance rules lis t Hive with no branch dialect in `CODE_OF_CONDUCT.md:135`. - **Not Reviewed Scope:** Runtime HiveServer2/Iceberg execution, full-repo `./mvnw clean install -B -T1C -Pcheck`, and GitHub Actions/CI status. - **Need Expert Review:** No specialized security, concurrency, or performance review required. Normal parser maintainer review is sufficient. ### Basis - **Root-cause fix:** The PR directly adds Hive grammar support for `ALTER TABLE ... EXECUTE DELETE ORPHAN-FILES` and the optional `OLDER THAN (...)` form, rather than adding fallback logic. The parser alternative is wired into `alterTable` and backed by `ORPHAN_FILES` / `OLDER` lexer tokens. - **Official syntax match:** The Iceberg Hive documentation explicitly documents both supported forms: no argument and `OLDER THAN ('timestamp')`. - **Identifier compatibility:** The previous blocker is addressed. `OLDER` is now included in `identifierKeywordsUnambiguous`, and the regression case `ALTER TABLE older EXECUTE DELETE ORPHAN-FILES;` is covered. This preserves the adjacent valid identifier path while enabling the new syntax. - **Blast-radius review:** The change is limited to the Hive dialect grammar and parser IT resources. Hive has no branch dialect in the project maintenance table, and no shared parser, binder, routing, metadata, SPI, dependency, or high-frequency JDBC/Proxy execution path is changed. - **Adversarial pass:** Cross-dialect regression path is not applicable beyond Hive. No config-disabled or feature-flag-off path exists for this grammar. The original symptom path is covered by both syntax variants, and the prior keyword-shadowing counterexample is now covered directly. ### Verification - Reviewer-run local verification on the PR head: `./mvnw -pl test/it/parser -am -DskipITs -Dspotless.skip=true -Dtest=org.apache.shardingsphere.test.it.sql.parser.hive.InternalHiveParserIT -Dsurefire.failIfNoSpecifiedTests=false test` exited `0`; `InternalHiveParserIT` ran 736 tests with 0 failures and 0 errors. - Reviewer-run style verification: `./mvnw -pl parser/sql/engine/dialect/hive,test/it/parser -am -Pcheck -DskipTests checkstyle:check` exited `0`. - Reviewer-run formatting verification: `./mvnw -pl parser/sql/engine/dialect/hive,test/it/parser -am -Pcheck -DskipTests spotless:check` exited `0`. - GitHub Actions / CI status was not inspected or used for this verdict. ### Multi-Round Comparison - Previous blank-line feedback in `DDLStatement.g4` is fixed. - Previous `OLDER` identifier compatibility blocker is fixed by `BaseRule.g4:337` plus the `alter_with_older_table` parser case in `test/it/parser/src/main/resources/sql/supported/ddl/alter-table.xml:379` and `test/it/parser/src/main/resources/case/ddl/alter-table.xml:2532`. - No unresolved previous-round blocker remains in the latest PR head. -- 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]
