morrySnow commented on PR #64026: URL: https://github.com/apache/doris/pull/64026#issuecomment-4715651449
## Code Review for PR #64026 整体方向正确，将 `PartitionValue` 重构为使用类型化的 `LiteralExpr` 是一个好的改进。以下是我关注到的几个关键问题： --- ### 🔴 1. `TimestampTzLiteral.getStringValue()` 潜在无限递归 **文件**: `fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/TimestampTzLiteral.java` 新的 `getStringValue()` 委托给 `toLegacyLiteral().getStringValue()`。如果 `toLegacyLiteral()` 内部（或通过 `DateLiteral.getStringValue()`）回调到 `TimestampTzLiteral.getStringValue()`，将导致无限递归和栈溢出。 请验证完整调用链：`TimestampTzLiteral.toLegacyLiteral()` → `DateLiteral` 的序列化路径是否会反向依赖 `getStringValue()`。 --- ### 🔴 2. `TypeCoercionUtils` 中的引用相等性检查 **文件**: `fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java`, 第 661 行 `dataType == TimeStampTzType.MAX` 使用了 Java 引用相等性检查。如果 `TimeStampTzType.MAX` 有多个实例（例如通过反序列化或 `DataType.fromCatalogType()` 创建），这个检查会失败，导致 scale 被错误地覆盖。 **建议**: 使用 `dataType.equals(TimeStampTzType.MAX)` 或 `((TimeStampTzType)dataType).getScale() < 0`。 --- ### 🟡 3. 错误消息误导 **文件**: `StepPartition.java:101`, `AlterMultiPartitionOp.java:122` 错误消息说 "partition column number in multi partition clause **must be one**"，暗示只支持单列分区。但 Doris 明明支持多列分区。实际含义是：边界值的数量不能超过分区列的数量。建议改为更清晰的表述，如： > "Number of partition boundary values (%d start, %d end) exceeds partition column count (%d)" --- ### 🟡 4. `PartitionValue.equals/hashCode` 语义变化 **文件**: `fe/fe-core/src/main/java/org/apache/doris/analysis/PartitionValue.java` `equals()` 和 `hashCode()` 现在包含了 `stringValue` 字段。两个具有相同 `LiteralExpr` 值但不同 `stringValue` 的 `PartitionValue` 对象现在会被视为**不相等**。请确认所有依赖 `PartitionValue.equals()` 的地方（如 partition 去重、比较）不会因此产生行为差异。 --- ### 🟡 5. DECIMAL 验证变得更严格 — 潜在的 breaking change **文件**: `fe/fe-core/src/main/java/org/apache/doris/analysis/LiteralExprUtils.java` 新的 `createDecimalLiteral` 使用 `RoundingMode.UNNECESSARY` 拒绝精度超过列 scale 的分区值。旧代码会静默截断/四舍五入。如果用户有分区值 `'10.001'` 对应 `DECIMAL(10,2)`（之前静默变为 `10.00`），现在会被拒绝。这是一个 behavior change，建议在 release notes 中提及。 --- ### 🟡 6. `DateTimeLiteral.roundMicroSecond` 对 TIMESTAMPTZ 的边界溢出正确性 **文件**: `fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/DateTimeLiteral.java` 新的 `roundMicroSecond` 使用 `LocalDateTime.of()` 处理微秒溢出，避免了 `getStringValue()` 的重新解析。对于 `TimestampTzLiteral`（继承自 `DateTimeLiteral`），构造函数在调用 `roundMicroSecond` 时参数已经是 UTC 时间——微秒进位导致日期时间字段向前推进时，是否仍然是正确的 UTC 时间？考虑 `9999-12-31 23:59:59.999999` 这种边界情况，year overflow to 10000 是否会导致 `LocalDateTime.of()` 抛出异常？ --- ### 🟢 7. `ScanNode.normalizePartitionFilterLiteral` — 建议增加日志 **文件**: `fe/fe-core/src/main/java/org/apache/doris/planner/ScanNode.java` 当 `createLiteral` 抛出异常时，方法静默返回原始值。建议添加 `LOG.debug()` — partition pruning 中的 normalization 失败是值得关注的事件，因为它会导致 `compareLiteral` 返回 -1 从而可能错过剪枝机会。 --- ### 🟢 8. `PartitionKey` 反序列化的类型保留 **文件**: `fe/fe-core/src/main/java/org/apache/doris/catalog/PartitionKey.java`, 第 535 行 类型保留逻辑在 `existingType.getPrimitiveType() != type` 时回退到 `Type.fromPrimitiveType(type)`。但如果 `existingType` 是一个 wildcard 类型（如 `VARCHAR(-1)`），其 `getPrimitiveType()` 可能匹配 `type`，导致参数化信息（长度）仍丢失。建议添加日志或在类型不完整时也回退。 --- ### 整体评价 - **compareLiteral 收紧**: ✅ 好方向。从抛异常/字符串兜底统一为返回 -1（视为不可比较），语义更清晰。与 `ScanNode.normalizePartitionFilterLiteral` 配合，实现了"先归一化再比较"的模式。 - **PartitionValue 类型化**: ✅ 好重构。减少了后期解析带来的 bug。 - **Nereids → catalog 类型转换**: ✅ 正确处理了 TIMESTAMPTZ 时区语义在分区边界传递中的一致性。 - **测试覆盖**: ✅ 增加了大量测试，覆盖了 TIMESTAMPTZ 时区场景和 compareLiteral 的新行为。特别是 `CreateTableLikeTest` 中的时区测试很全面。 - **代码风格**: 将 `stream().collect()` 替换为显式 `for` 循环，提高了可读性和调试友好性。 总体来看，这是一个质量较高的重构，方向正确，测试覆盖充分。上述几点主要是需要确认和关注的风险点。 --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
