github-actions[bot] commented on PR #61918: URL: https://github.com/apache/doris/pull/61918#issuecomment-4161899293
## Code Review Summary This is a large-scale refactoring PR (125 files, +1500/-1171) that decouples FE module dependencies across three main areas. The refactoring is well-executed with thorough migration of all call sites and no functional regressions detected. ### Critical Checkpoint Conclusions **1. Goal and Correctness:** The PR achieves its stated goal of decoupling `SlotRef`/`Column`/`GlobalVariable` dependencies to enable cleaner module separation. All callers have been migrated — no orphaned references to removed methods (`Column.toThrift()`, `Column.toPb()`, `Expr.getExprName()`, `TableNameInfo(TableIf)`, `TableNameInfo.createOrNull()`, `Env.isStoredTableNamesLowerCase()`, `VariableMgr.VarAttr`, etc.). **2. Modification Focus:** The PR bundles three logically independent refactorings (Part A: TableNameInfo/GlobalVariable, Part B: Column serialization, Part C: Expr visitor). While each is internally coherent, they could have been separate PRs for easier review. Acceptable for a refactoring PR. **3. Concurrency:** No new concurrency concerns introduced. The moved `GlobalVariable` fields retain the same `volatile` / static semantics. `NameSpaceContext` is immutable and thread-safe. **4. Lifecycle / Static Initialization:** The `GlobalVariable.resolveSystemTimeZone()` static initializer uses only JDK `TimeZone.getDefault().getID()`, avoiding cross-module static dependency. The CST→Asia/Shanghai mapping is a simplification from the original `TimeUtils.getSystemTimeZone()` which used a full alias map via `ZoneId.of()`, but is adequate for system timezone initialization where the JVM reports a proper IANA zone ID. **5. Serialization Compatibility:** The `Column.fieldPatternType` field changes from `TPatternType` to `PatternType` enum. Both enums have identical constant names (`MATCH_NAME`, `MATCH_NAME_GLOB`), so GSON serialization/deserialization remains backward compatible (GSON serializes enums by name). **6. Functionally Parallel Paths:** All call sites for `Column.toThrift()` (12 sites), `Column.toPb()` (1 site), `setIndexFlag()` (4 sites), and `getExprName()` (4 sites via visitor) have been migrated. Verified via comprehensive search. **7. Configuration:** No new configuration items added. Existing configuration behavior preserved. **8. Test Coverage:** `ConstraintTest.java` has 9 test methods covering constraint operations with the refactored `TableNameInfo`. `CreateTableCommandTest.java` verifies the `ExprToExprNameVisitor`. `SessionVariablesTest.java` verifies `VarAttrDef` migration. Tests are adequate for a refactoring PR. **9. Incompatible Changes:** No incompatible changes. `InternalCatalog.INTERNAL_CATALOG_NAME` is preserved as a deprecated re-export from `NameSpaceContext.INTERNAL_CATALOG_NAME`, maintaining backward compatibility. **10. Build Changes:** The gensrc plugin and `Version.java` generation are correctly moved from `fe-core` to `fe-common` pom.xml, with corresponding changes in `gen_build_version.sh` and the Makefile. ### Minor Observations (Non-blocking) - **Redundant pre-lowering at call sites:** Several call sites (e.g., `AddConstraintCommand`, `DropConstraintCommand`, `ShowConstraintsCommand`, `InternalCatalog`, `MTMVPartitionUtil`) explicitly call `toLowerCase()` on table names before passing to `TableNameInfo(ctl, db, tbl)`, but the constructor already handles this internally. This is harmless (idempotent) but creates inconsistency with other call sites (like `ForeignKeyConstraint`) that don't pre-lower. Consider unifying the style in a follow-up. - **Null safety pattern inconsistency:** `ForeignKeyContext.putAllForeignKeys()`/`putAllPrimaryKeys()` use explicit null guards for `getDatabase()`/`getCatalog()` (defensive style), while `AddConstraintCommand`, `ShowConstraintsCommand`, and `ForeignKeyConstraint` do not guard against null. The risk is very low since tables are freshly resolved, but the pattern is inconsistent. The `MTMVService.shouldRefreshOnBaseTableDataChange()` also uses defensive null checks with try/catch — this is reasonable for an event handler where availability matters more than failing fast. **Overall: No critical or blocking issues found. The refactoring is clean and complete.** -- 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]
