luwei16 commented on PR #66963: URL: https://github.com/apache/doris/pull/66963#issuecomment-5794222498
## Local re-review: PASS Reviewed the complete 21-file three-dot diff for head `17b98f167df5afadaeb0aedb472773b8c1a22f6f` against base `53e1f62096e85fda10d24dd7ef65547ecdc694d0`. One review round with two full-review subagents, one additional risk-focused subagent, independent main-agent verification, and a final complete sweep converged with **0 Blocker, 0 Major, 1 Minor, 0 Nit**. Both previously reported Major findings are resolved: - The internal-table exemption now uses the resolved internal catalog and `__internal_schema` identity instead of the sticky `QueryState.isInternal` flag. The new FE integration test covers three state types across DUPLICATE and UNIQUE tables with that flag already set. - The restriction regression now reconnects to the master FE and holds the same global configuration lock as the compatibility suites throughout configuration capture, negative tests, positive tests, and restoration. Nested temporary settings restore false first, then the original value. ### Remaining non-blocking Minor [AGG_STATE CREATE regression, line 57](https://github.com/apache/doris/blob/17b98f167df5afadaeb0aedb472773b8c1a22f6f/regression-test/suites/data_model_p0/test_state_types_only_in_aggregate_table.groovy#L57): the DUPLICATE-table declaration includes `GENERIC`, so `CreateTableInfo.validateKeyColumns` rejects the aggregate clause before reaching the new state-type restriction. This case would also pass without the new restriction. Remove `GENERIC`, assert the new restriction diagnostic, and add the corresponding successful AGG_STATE CREATE with the compatibility configuration enabled. The new FE integration test already covers the valid declaration and default rejection, so this coverage gap is not blocking. ### Critical checkpoints | Checkpoint | Conclusion | |---|---| | Goal and evidence | Ordinary non-AGG definitions reject all three state types by default; FE tests cover the core matrix and exemptions. | | Scope and abstraction | Validation is centralized in ColumnDefinition, with explicit exemption call sites and related test adaptations. | | Concurrency and locks | No new production locks; configuration-sensitive regression suites share one named lock, with no new lock-order cycle. | | Lifecycle | Configuration and session state are restored; replaced JDBC connections are closed and the active connection remains suite-managed. | | Dynamic configuration | Mutable/master-only; affected DDL is forwarded before execution and validation, and reads the setting on each validation. | | Compatibility | Intentional new-DDL restriction has a temporary compatibility switch; no storage-format or protocol changes. Existing data access does not use the new check. | | Parallel paths | CREATE, LIKE, CTAS, ADD COLUMN(S), and MODIFY use the common validation; existing nested-type rules already exclude these state types. | | Special conditions | Internal statistics tables use resolved catalog/database identity; MTMV creation, refresh, and IVM share the explicit exemption. | | Test coverage | Core type/model/configuration matrix, internal statistics table, MV, and sticky-state rejection are covered; the Minor above remains. | | Expected-result files | No .out changes; new regression checks DDL success/failure. | | Observability | The new error names the type, column, and compatibility setting; no additional metrics are necessary. | | Transactions and persistence | No EditLog, replay, transaction, or failover-protocol changes. | | Data writes and atomicity | Rejection occurs during DDL validation, before metadata creation/change; no new BE write or crash-recovery path. | | FE/BE parameter propagation | No new transmitted fields. | | Performance | Constant-time checks per column on DDL validation, with no query hot-path cost. | | Other issues | Complete final sweep found no additional substantiated findings. | No builds or tests were run during this read-only re-review. Previously produced logs were inspected: **51 FE unit tests passed, with zero Checkstyle violations**. The new rejection test failed against the old implementation as expected. Live regression execution remains unverified. <!-- doris-repo-review:v1:begin --> ```yaml schema: doris-repo-review/v1 status: PASS pr: apache/doris#66963 commit: 17b98f167df5afadaeb0aedb472773b8c1a22f6f base: 53e1f62096e85fda10d24dd7ef65547ecdc694d0 reviewed_at: 2026-09-23T07:57:19+00:00 reviewer: luwei16 model: gpt-6-astra effort: xhigh findings: {blocker: 0, major: 0, minor: 1, nit: 0} rounds: 1 converged: true ``` <!-- doris-repo-review:v1:end --> -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
