terrymanu commented on PR #39622: URL: https://github.com/apache/shardingsphere/pull/39622#issuecomment-5448930442
### Result **Review Result: Mergeable** PR #39622 correctly restricts the Doris parser to syntax supported by Apache Doris while preserving valid inline window functions and supported `DISTINCT` analytic aggregations. ### Evidence - Reviewed head: [`e48969a0404ff909e7f791a53d5debb4195f4fb9`](https://github.com/apache/shardingsphere/commit/e48969a0404ff909e7f791a53d5debb4195f4fb9), based on `08369e3cc2d27b5ae167f302f624d63f35923590`. - Apache Doris 4.1.3 is the current release. Its [official grammar](https://github.com/apache/doris/blob/7126cf65d96ebc43fce0906f51e92c1a2ccf24a6/fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4#L1797-L1806) supports only inline `OVER (...)` specifications and explicitly leaves named-window references unsupported. - Doris’s [official parser implementation](https://github.com/apache/doris/blob/7126cf65d96ebc43fce0906f51e92c1a2ccf24a6/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java#L3516-L3524) allows `DISTINCT` analytic calls only for `COUNT`, `SUM`, and `GROUP_CONCAT`. - `BaseRule.g4:1104-1183` enforces those two boundaries, while `DMLStatement.g4:156-160` removes the unsupported query-level `WINDOW` clause. - The visitor changes consistently remove unreachable named-window AST paths and preserve inline window metadata. - Positive coverage preserves non-window `MAX(DISTINCT ...)`, `MAX(ALL ...) OVER (...)`, and the three supported `DISTINCT` analytic functions. - Negative coverage at `test/it/parser/src/main/resources/sql/unsupported/unsupported.xml:34-42` exercises five named-window forms and four unsupported `DISTINCT` analytic functions. - The originating [Doris parser issue](https://github.com/apache/shardingsphere/issues/31506) explicitly warns that scraped syntax may be incorrect and requires validation against the actual database, which supports this correction. - Verification passed: - Doris parser package: exit 0, `BUILD SUCCESS`. - `InternalDorisParserIT` and `InternalUnsupportedDorisParserIT`: 1,419 tests, 0 failures, 0 errors. - Scoped Spotless and Checkstyle: exit 0, 0 violations. - `git diff --check`: exit 0. ### Coverage - All 11 authoritative GitHub files match the local triple-dot scope and were reviewed. - Behavior clusters: named-window rejection, `DISTINCT` analytic allowlist, visitor/AST convergence, and supported/unsupported fixture alignment. - MySQL trunk and Hive sibling behavior were checked; their named-window support remains unchanged, and shared test cases retain their respective dialect assignments. - Apache Doris 4.1.3 documentation and source, repository parser examples, generated grammar consumers, removals, functional regressions, and parser-cost risk were reviewed. - Root-cause, blast-radius/contracts, tests/runtime, and final convergence passes completed with no unresolved findings. - The PR head was rechecked after verification and remained unchanged. - This is a code-scope review only; CI was not reviewed. -- 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]
