Zhengcy05 commented on PR #38829: URL: https://github.com/apache/shardingsphere/pull/38829#issuecomment-4743215135
> ### Summary > * **Merge Decision: Not Mergeable** > * **Reason:** The MySQL fix is in the right direction, but the shared-kernel guard and SQL-parser family scan leave Doris temporary-table DDL unflagged and unblocked on the same metadata-refresh class of path. > > ### Positive Feedback > * Preserving the MySQL `TEMPORARY` semantic in the visitor, statement model, binder, and pre-execution kernel check directly targets the reported Proxy failure mode from #38709. > > ### Issues > * **P1 Doris temporary-table DDL still bypasses the temporary-table guard** (`infra/context/src/main/java/org/apache/shardingsphere/infra/connection/kernel/KernelProcessor.java:74`) > > * **Problem:** The new guard returns unless `database.getProtocolType()` is MySQL, while the shared statement model now has a generic `temporary` flag. Doris is a MySQL-family branch parser in the repo conventions: its grammar accepts `CREATE TEMPORARY TABLE` and `DROP TEMPORARY TABLE`, but `DorisDDLStatementVisitor` still builds `CreateTableStatement` without `.temporary(...)` and builds `DropTableStatement` with the legacy constructor that defaults the flag to `false` (`parser/sql/engine/dialect/doris/src/main/java/org/apache/shardingsphere/sql/parser/engine/doris/visitor/statement/type/DorisDDLStatementVisitor.java:633`, `parser/sql/engine/dialect/doris/src/main/java/org/apache/shardingsphere/sql/parser/engine/doris/visitor/statement/type/DorisDDLStatementVisitor.java:1247`). Apache Doris 4.x docs describe temporary tables as session-bound, session-visible, same-name-shadowing tables, and `PushDownMetaDataRefreshEngine` still treats all `CreateTableStatement` / `DropTableSt atement` instances as refreshable (`mode/core/src/main/java/org/apache/shardingsphere/mode/metadata/refresher/pushdown/PushDownMetaDataRefreshEngine.java:61`). > * **Impact:** A Doris `CREATE TEMPORARY TABLE` can still be parsed with `temporary=false`, bypass the new shared guard, execute, and enter the normal push-down table metadata refresh path where `CreateTablePushDownMetaDataRefresher` unconditionally loads and persists the created table (`mode/core/src/main/java/org/apache/shardingsphere/mode/metadata/refresher/pushdown/type/table/CreateTablePushDownMetaDataRefresher.java:40`). That leaves a related dialect with the same session-scoped / shadowing temporary-table risk unresolved, and it also places target-dialect logic in a shared high-frequency kernel class without a non-MySQL counterexample proving the generic contract. > * **Required Change:** Please either preserve the `temporary` flag and reject temporary-table DDL for every affected parser/protocol family that shares this runtime risk, including Doris, or add public evidence and regression tests proving Doris is not applicable to this runtime path. Please also keep the shared `KernelProcessor` contract generic, or cover the intentional MySQL-only behavior with explicit non-target dialect tests. > > ### Review Details > * **Reviewed Scope:** Latest PR head `c803e13d5973dd8a36ba7d45c0adb369aab2cff3`; local merge-base `1e890783241fe4de1b781d3feb945b687d526205`; GitHub `/pulls/38829/files` matched the local `git diff <merge-base>..refs/remotes/apache/pr/38829` file list after normalizing status labels. Reviewed the 9 PR files under `infra/binder/core`, `infra/context`, `parser/sql/engine/dialect/mysql`, `parser/sql/statement/core`, and `proxy/backend/core`, plus related Doris parser and push-down metadata refresh paths. SQL syntax/semantics checked against MySQL 8.0 docs for [`CREATE TEMPORARY TABLE`](https://dev.mysql.com/doc/refman/8.0/en/create-temporary-table.html) and [`DROP TABLE`](https://dev.mysql.com/doc/refman/8.0/en/drop-table.html), and Apache Doris 4.x [`Temporary Table`](https://doris.apache.org/docs/4.x/table-design/temporary-table/) docs. MariaDB has no separate parser directory under `parser/sql/engine/dialect` in this tree; Doris has a separate branch parser and was checked direc tly. > * **Not Reviewed Scope:** CI/check-runs were intentionally not reviewed. I did not run a full Proxy startup, E2E scenario, or exhaustive documentation/release-note sweep after finding the code-level blocker. > * **Verification:** Reviewer-run evidence collection used public GitHub REST PR/files/reviews/comments/issue endpoints, all exit `0`; `git fetch apache master:refs/remotes/apache/master pull/38829/head:refs/remotes/apache/pr/38829` exit `0`; local diff/file-boundary comparison exit `0` and matched GitHub scope. Maven tests were not run because the merge-blocking parser-family/shared-layer issue is visible from the latest diff and related source paths. > * **Release Note / User Docs:** Not reviewed as a blocker in this pass; after the code blocker is resolved, please re-check whether a bug-fix release note is needed for the user-visible Proxy behavior change from internal NPE/backend execution to deterministic unsupported-SQL rejection. Thanks for the detailed review. I have updated the fix in the following way: - preserved the `TEMPORARY` flag in Doris DDL visitor for both `CREATE TEMPORARY TABLE` and `DROP TEMPORARY TABLE` - removed the MySQL-only branch in `KernelProcessor`, so the pre-execution guard now rejects temporary-table DDL based on the shared statement model instead of protocol type - added Doris parser regression tests to verify the temporary flag is preserved - updated kernel tests to verify the shared guard behavior is not limited to MySQL and still cannot be bypassed by `skipMetadataValidate` -- 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]
