sundapeng commented on PR #8728: URL: https://github.com/apache/paimon/pull/8728#issuecomment-5016511494
## 🔍 DLF-Review 自动代码审查 (GAN) **模式**: FULL | **Score**: 78/100 **严重度分布**: major: 1, minor: 7, suggestion: 5 **GAN Stats**: 17 原始 → 14 去重 → Disc(8✓ 5? 1✗) → Arb(+5) → 13 最终 ### 代码问题 (8) 🟠 **major (1)** - **PartitionPathUtils.java:201** — 转义/未转义不匹配：buildPartitionNamePrefixPattern 生成转义形式的 LIKE 模式，但 catalog 端（PartitionUtils.buildPartitionName）使用未转义形式匹配，导致含特殊字符的分区值静默返回空结果集 🟡 **minor (4)** - **FormatTable.java:330** — FormatTable（@Public API）向上依赖 CatalogUtils，形成 table↔catalog 包级循环依赖 - **PartitionPathUtils.java:459** — 严格 key 匹配使分区键重命名/大小写不同的旧目录静默不可见 - **RESTCatalog.java:578** — catalog 级 table-default 选项参与 managed format table 校验，可能破坏现有 CREATE/REPLACE TABLE 工作流 - **PartitionPathUtils.java:398** — value-only 分区值新增反转义，改变现有 format table 的分区 spec 行为 🟢 **suggestion (3)** - **FormatTableCatalogProvider.java:74** — Catalog 实例无 close/lifecycle 契约，未来非 REST catalog 可能泄漏资源 - **FormatTableCommit.java:248** — 分区 spec 提取逻辑在 FormatTableCommit 和 PartitionPathUtils 间重复 - **ManagedFormatTableScan.java:80** — 每次 managed scan 为每个分区构建转义路径字符串仅用于去重，存在冗余 escape/validate 开销 ### 代码风格与文档 (advisory, 5) - **FormatTableCatalogProvider.java:110** — 注释引用 MSCK（Hive 概念），Paimon 中应引用 sys.repair - **FormatTableCommit.java:230** — 错误消息引用不存在的 sys.sync_format_table_metadata 和 MSCK REPAIR TABLE，应改为 CALL sys.repair(...) - **ManagedFormatTableScan.java:58** — 与 FormatReadBuilder 重复的 catalogProvider null 检查，错误消息不一致 - **ManagedFormatTableScan.java:128** — warnIfFilesystemPartitionsExist 日志引用不存在的 sys.sync_format_table_metadata 和 MSCK REPAIR TABLE - **ManagedFormatTableScan.java:203** — onPartitionFileNotFound 警告引用不存在的 sys.sync_format_table_metadata 和 MSCK REPAIR TABLE ### 👍 亮点 - Excellent documentation and graceful degradation strategy: the MR adds clear user-facing docs (sql-ddl.md) explaining the managed format table feature, and implements a thoughtful fallback (warning + filesystem discovery) for tables loaded in catalogs that cannot honor the managed option, rather than refusing to load them - preserving backward compatibility while enabling the new feature. - Careful commit/abort semantics with data safety: the partitionRegistrationStarted flag and the decision to preserve data files when partition registration fails (ambiguous outcome) demonstrates strong distributed-systems thinking. The error message includes actionable recovery guidance, and the finally-block generation advancement in createPartitions/dropPartitions handles the case where the server committed but the response was lost. - Well-designed cache invalidation architecture: the static GENERATIONS counter with Caffeine TTL in FormatTableCatalogProvider provides process-local immediate invalidation after mutations while allowing cross-JVM convergence via TTL expiry. The generation-keyed cache design avoids stale reads within a JVM without requiring distributed coordination, and the bounded cache sizes (MAX_CACHED_PATTERNS, MAX_TRACKED_TABLE_GENERATIONS) prevent unbounded memory growth. --- _Qoder DLF-Review (GAN)_ -- 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]
