Skip to content

fix(plan): separate shuffle admission from filter cardinality - #26791

Draft
aptend wants to merge 1 commit into
matrixorigin:mainfrom
aptend:fix/optimizer-join-cardinality-stats
Draft

fix(plan): separate shuffle admission from filter cardinality#26791
aptend wants to merge 1 commit into
matrixorigin:mainfrom
aptend:fix/optimizer-join-cardinality-stats

Conversation

@aptend

@aptend aptend commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #26742

What this PR does / why we need it:

A residual FILTER keeps the existing 5% point estimate for cardinality and join ordering. That estimate is not a safe memory bound when one predicate references columns from multiple relations, because marginal column statistics cannot estimate their correlation.

This change separates the two decisions. Cardinality and join order remain unchanged. For shuffle admission only, a valid shuffle candidate may use the FILTER input cardinality when the build-side predicate spans relations and the input exceeds the existing large-build threshold. Small filters, single-relation predicates, invalid shuffle keys, and post-remap plans keep the existing behavior.

This preserves stable benchmark plans while keeping the uncertain Q64 build side eligible for partitioned execution and spill.

@aptend
aptend requested review from XuPeng-SH and aunjgr as code owners August 7, 2026 11:10
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label Aug 7, 2026
@mergify mergify Bot added the kind/bug Something isn't working label Aug 7, 2026
@matrix-meow matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/XXL Denotes a PR that changes 2000+ lines labels Aug 7, 2026
@aptend
aptend requested a review from heni02 as a code owner August 7, 2026 16:29

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REQUEST_CHANGES

本次审查基于 base c72224b 与 head 43fb4cd,结论是当前实现仍不适合合入。

P1: ReCalcNodeStats 引入近似 O(J^3) 的重复遍历和大量分配

uniqueColsInSubtree 会递归遍历整棵 join 子树;同一个 INNER JOIN 又多次调用 getEquiJoinKeyPairs、唯一性、containment 等分析。优化流程还会重复执行多轮 ReCalcNodeStats

同机、相同工具链的左深 join 基准:

  • 20 表:base 0.31 µs / 0 alloc;PR 207 µs / 203 KB / 8,589 alloc
  • 40 表:base 0.69 µs / 0 alloc;PR 1.59 ms / 2.34 MB / 61,234 alloc
  • 80 表:base 1.89 µs / 0 alloc;PR 13.83 ms / 29 MB / 448,805 alloc

这不是微优化问题;复杂查询的规划阶段会额外产生数百 MB 临时分配。应在一次 join analysis 中缓存 tags、equality pairs、列集合和唯一性结果,并在统计重算中复用。

P1: 过滤复合唯一键的估算没有可证明的前置条件,仍可能低估 build side

estimateFilteredCompositeUniqueJoinCardinalityStats.Selectivity 当作“保留复合键比例”。但 Stats.Selectivity 在 FILTER、JOIN 和子树中分别表示不同语义,不能证明键域覆盖率。

反例:unique composite side 从 100k 行过滤到 20k 行,probe 为 1m 行且最大单列 NDV=50k;如果 900k probe 行集中落在这 20k 个保留键上,实际 join 是 900k,而当前估算会从 400k 再截到 200k。该误差会影响 build/shuffle 决策并可能重新触发 #26742 的 HashBuild OOM。

此外,0.99 containment 阈值会产生不连续跳变:在 PR 测试参数下,probe max 从 1010 改为 1011 就可能从 1m 直接降到 1k。应移除基于全局选择率的复合键 cap;如果没有 tuple-domain/frequency 统计,应保留保守上界或保持 shuffle eligibility。

P2: 新增了一套与 determineHashOnPK 重复的 PK/唯一性传播逻辑

本 PR 的 uniqueColsInSubtree 与已有 determineHashOnPK 都在做 tags、equality keys、PK 等价传播。两套实现已经不一致:新实现声称支持 PROJECT,但没有把输出列映射回 ProjectList;已有实现也不会复用这套 lineage。

这会导致后续 join/projection 语义变更需要维护两套规则,同时也是当前重复遍历和性能退化的设计根因。建议抽取窄的只读 relational-key analysis,由 cardinality 和 HashOnPK 共同消费,不要继续增加第三套属性框架。

P2: PROJECT 唯一性和 NDV lineage 未闭环

Node_PROJECT 直接把 (RelPos, ColPos) 原样传给 child。普通 project 会产生新 binding tag,因此 scan tag=1 PK(k) -> project tag=2 output[0]=tag1.k 的唯一性证明会失败;getExprNdv 也无法通过 project tag 找到基础统计。该路径不会产生错误结果,但会让本 PR 的核心优化在常见子查询/CTE/别名投影下失效。

应仅对直接列投影做安全 lineage 映射;计算表达式保守拒绝,并补充 projection、重排、别名和 project-over-join 测试。

测试与维护性

新增约 647 行测试,其中 Q64 fixture 手工复制具体表、采样浮点数和当前 join 形状,并断言精确中间基数;这更像 incident snapshot,而不是稳定的不变量测试。TestFilteredCompositeUniqueJoinUsesRetainedTupleRatio 还没有真正进入对应 helper 分支。建议拆成小型 table-driven 边界/偏斜测试,再保留一个真实 planner 路径只验证 build side、shuffle 和语义结果,不冻结当前公式的精确浮点数。

建议先收敛为:单次 join analysis + 明确的 cardinality-local NDV 约束 + 可复用的 PK/lineage 属性;删除无法证明的选择率 cap 和重复的特例分支。

@aptend aptend changed the title fix(plan): make join cardinality estimates constraint-aware fix(plan): estimate standalone filter selectivity Aug 10, 2026
@aptend

aptend commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

已按 review 收缩方案,当前最终 diff 只剩 stats.go 7 行新增/3 行删除和一个 56 行通用测试:

  • 删除复合唯一键 selectivity cap、0.99 containment 阈值及全部 join-NDV 特例;
  • 删除递归 uniqueColsInSubtree,因此 O(J^3)、重复 PK 分析和 PROJECT lineage 问题随之消失;
  • 删除 Q64 大型精确浮点 fixture;
  • 仅修复可证明的问题:Standalone FILTER 不再固定估算 5%,而是复用现有表达式选择率估算器,并在深拷贝上计算,避免污染物理规划谓词;
  • 边界测试同时覆盖等值谓词保持 resident、非选择性不等谓词保持 shuffle/spill eligibility。旧实现会在不等谓词 case 得到 5% 且 shuffle=false,新实现得到 90% 且 shuffle=true

未在本 PR 中继续猜测日期域相关性、复合键 overlap 或 tuple NDV。

@aptend
aptend requested a review from XuPeng-SH August 10, 2026 02:17
@matrix-meow matrix-meow added size/S Denotes a PR that changes [10,99] lines and removed size/XL Denotes a PR that changes [1000, 1999] lines labels Aug 10, 2026
@aptend
aptend marked this pull request as draft August 10, 2026 02:31
@aptend
aptend force-pushed the fix/optimizer-join-cardinality-stats branch from 61bf54d to 4a4df16 Compare August 10, 2026 02:49
@aptend aptend changed the title fix(plan): estimate standalone filter selectivity fix(plan): separate shuffle admission from filter cardinality Aug 10, 2026
@aptend

aptend commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

已用本地 SF1000 fixture 重新验证并替换上一版方案:不再修改 FILTER selectivity,也不改 cardinality 或 join order;最终 diff 仅在 shuffle admission 阶段为大型跨关系残余过滤保留输入侧风险上界。99 条 TPC-DS 中实质物理变化仅为 Q64 两个对称 join 开启 range shuffle;22 条 TPC-H 无物理变化。通用边界测试覆盖单关系、小型跨关系和大型跨关系三类。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants