luwei16 commented on PR #66963: URL: https://github.com/apache/doris/pull/66963#issuecomment-5794565617
## Local re-review after configuration rename: PASS The requested configuration rename to `enable_non_aggregate_table_state_types` is complete in the declaration, validation error, FE tests, regression settings, and all 12 shared-lock users. The new commit contains only 37 consistent name replacements across 17 files; default false, mutable/master-only scope, and the temporary-removal notice are unchanged. The PR description also uses the new name. Reviewed the complete 21-file three-dot diff for head `f99d9080ccb15753285d4119d6a24bdfeb2a082b` 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/f99d9080ccb15753285d4119d6a24bdfeb2a082b/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. | Validation of this implementation was completed before the read-only review: **52 focused FE unit tests passed, with zero failures/errors/skips**, using `run-fe-ut.sh`. Checkstyle passed for both `fe-common` and `fe-core` with zero violations, and `git diff --check` passed. This covers CreateTableTest, ColumnDefinitionTest, InternalSchemaAlterTest, MTMVPlanUtilTest, and the HLL Bloom Filter case. No further builds or tests were run during the review itself. Full product packaging and live SQL regression remain unverified locally. <!-- doris-repo-review:v1:begin --> ```yaml schema: doris-repo-review/v1 status: PASS pr: apache/doris#66963 commit: f99d9080ccb15753285d4119d6a24bdfeb2a082b base: 53e1f62096e85fda10d24dd7ef65547ecdc694d0 reviewed_at: 2026-09-23T12:11:55+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]
