englefly commented on PR #65846: URL: https://github.com/apache/doris/pull/65846#issuecomment-5224250852
改进意见 1. **PR 描述/标题与 diff 严重不符 (最重要)**。标题和 description 只讲多参数聚合函数, 但 diff 包含上面列出的约 9 项独立行为变更。每一项都值得单独 review, 建议在 description 里完整列出 (或拆成多个 PR), 否则 reviewer 无法逐项核对, release note 也容易漏写。 2. **标题措辞与实情有出入**。all-key distinct 的多参数聚合在旧代码里本来就是 ON (只是碰巧依赖 child(0)、且与参数顺序相关); 本 PR 真正做的是"含值列的多参数聚合必须 OFF, 且判定与参数顺序无关"。建议标题改为类似 "reject pre-agg for multi-argument aggregates with value-column inputs" 或 "fix order-dependent preagg decision for multi-arg aggregates"。 3. **cast 全面拒绝偏保守, 有性能回退风险**。整数加宽 cast (BIGINT→DECIMAL(20,0)、BIGINT→LARGEINT、INT→BIGINT) 是保序单射, max(cast(x)) == cast(max(x)) 严格成立, 不存在 -0.0 问题。现在 q26/q27 把"一律 OFF"固化进了断言, 意味着 `select max(cast(v9 as decimal(20,0)))` 这类查询将永久失去 preagg。建议对 MAX/MIN 白名单"精确加宽"的整型 cast, 仅拒绝浮点/收窄 cast。 4. **集中式 volatile 检查粒度过粗**。PreAggInfoContext 是整个 aggregate 子树共享的, 现在任何一个 filter/join/agg/grouping 里的 volatile 都会让该 aggregate 下所有 scan 全部 OFF。正确但过于保守 (比如只有 B 表的 filter 里有 random(), A、C 表也被关掉)。建议把 volatile 检查限定在"与本 scan output 有 slot 交集"的表达式上, 与现有 value-slot 栅栏一致; 至少补注释说明这是有意为之。 5. **`checkAggregateFunctions` 里 `if (aggSlots.isEmpty())` 分支不可达**。candidateAggFuncs 的构建已保证与 outputSlots 交集非空, 该分支是死代码, 建议删除或加注释, 避免误导后续维护者。 6. **建议补 FE 单测**。这次的分派矩阵 (纯 key 多参 / 混合多参 / 纯 value IF 路径 / volatile / ownership) 逻辑密集, 目前只有 explain 回归测试覆盖; 建议对 `checkAggregateFunctions` / `checkAggWithKeyAndValueSlots` 补 Java 单测 (构造表达式集合直接断言 PreAggStatus), 防止后续重构悄悄破坏。 7. **count(distinct) 也可豁免 ownership 栅栏 (小优化)**。栅栏只豁免 MAX/MIN, 但 `count(distinct ...)` 对重复值天然免疫 (distinct 去重), 外键返回值重复计入不会改变计数, 当前会被误伤为 OFF。可考虑把 distinct Count 也加入豁免。 8. **explain 断言依赖精确 reason 字符串** (如 "can't turn preAgg on because the scan is the selected side of an ASOF join"), 消息一改测试就挂; 与现有套件惯例一致, 可接受, 但建议保持 reason 文案稳定。 9. 小点: value-only 分支的注释 "only value slots" 实际还包括"全部为无 OriginalColumn 的派生 slot" (splitKeyValueSlots 会丢弃未分类 slot), 注释可更准确; PR body 的 Issue Number 还是 "close #xxx" 占位、Test 勾选框全部未勾, 建议补齐; 17 个 commit 里 12 个是 "fix comment", merge 时建议 squash。 -- 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]
