Skip to content

perf: avoid metadata simulation for ordinary fixed MIN/MAX values - #31006

Open
LeftHandCold wants to merge 2 commits into
matrixorigin:mainfrom
LeftHandCold:codex/fixed-minmax-default-preflight-20261024
Open

LeftHandCold wants to merge 2 commits into
matrixorigin:mainfrom
LeftHandCold:codex/fixed-minmax-default-preflight-20261024

Conversation

@LeftHandCold

@LeftHandCold LeftHandCold commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

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

Which issue(s) this PR fixes:

Fixes #31005.

What this PR does / why we need it:

Fixed MIN/MAX capacity preflight repeatedly simulates metadata winners even when ordinary input and destination vectors cannot create metadata sidecars. Skip that simulation only when both sides have default prepare/string-source metadata and nil sidecars. Retain input bounds, original malformed-target errors, active/standby target checks and the existing fallback for any metadata-bearing vector. BatchFill, merge, capacity admission, spill, cleanup and value comparisons are unchanged. The 39-line production change adds no allocation or retained state.

On original TSBS CPU-only data (8.64 million measurements, 1,000 tags), A/B/B/A with ten measured samples per version/query gives:

Query Baseline median Candidate median Whole-unit CPU change
Q1 hourly MAX 298.39 ms 225.25 ms -30.72%
Q5 minute MAX 355.63 ms 239.70 ms -34.73%

All five original queries retain complete source-checked outputs and identical physical objects. Q2/Q3/Q4 control wall changes are -1.03%/+15.60%/+3.65%, with CPU +0.47%/+2.68%/-0.02%. Q3 is 39.01→45.10 ms with intermittent 63–80 ms samples in both versions; this run does not establish that every control is regression-free. Raw samples are retained and Q3 remains a control in the combined-patch validation. Whole engine/client/validator resources are four CPU cores and 12 GiB without swap. Whole-block memory peaks are 1.70–1.88 GiB, with zero OOM/limit events. This causal A/B isolates only the preflight patch on b78794175e65; it does not combine DateTrunc or Top-1 gains.

Validation:

  • Baseline and candidate pass 16 targeted tests covering real fill results, NULL/constant/offset/256-row boundaries, malformed target shape with unchanged error/no-mutation behavior, ordinary seen metadata, active/standby state, exhausted budgets, target-only metadata and spill decoding. A deliberately disabled metadata guard fails the capacity assertion.
  • Complete owning aggexec package and dependent Group capacity/spill/cancellation/reuse tests pass. Scoped vet, configured incremental golangci-lint, formatting, diff and native-provenance checks pass on the causal candidate.
  • Thirteen paired micro controls: ordinary preflight -79.2% to -92.1%; preflight plus fill -58.1% to -69.0%; metadata fallback -0.83%/-0.67%; unchanged zero allocations.
  • Delivery commit 08cf92dcbb55 is based on verified main 1651eec948; all four changed source/test files are byte-identical to the validated causal candidate. The full owning package, seven current-main Group capacity/spill/cancellation/reuse consumers, scoped vet/lint, formatting/diff and native-provenance checks pass on this clean delivery head.

This is a draft for review. CPU-only results establish this improvement; broader benchmark and combined-patch validation remain separate work.

@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 →

@LeftHandCold LeftHandCold left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

代码复核:建议通过,未发现需要 Request Changes 的确定性问题。审查 HEAD:08cf92dcbb550611677f9e835b845c5028d758e9。

设计判断:在现有容量预检 owner 内消除普通值的重复元数据模拟;没有新增容量账本,也没有绕过 GroupGrow/BatchFill/spill 的既有责任。

逐项核对:

  • 快路径同时要求输入与每个实际触及的目标没有非默认 prepare/string-source 元数据,且 sidecar 为 nil;“非 nil 但长度为 0”的 sidecar 不会误入快路径。
  • 原输入范围/常量广播、GroupNotMatched、NULL、active/standby 容量检查保留。目标形状异常或任一目标带元数据会回退原预检。
  • 对照了实际 BatchFill 的 metadata setter:普通值对普通目标不会创建这些 sidecar;不是仅检查输入就跳过目标的潜在分配。
  • 新测试覆盖拒绝分配后的不发布、目标-only 元数据、后续 chunk、空 sidecar、standby、spill/merge 和消费侧容量边界。

非阻断的性能说明:作者报告 Q3 对照 wall time 39.01→45.10ms(+15.60%),CPU +2.68%,两边均有慢样本。现有证据不足以认定本 PR 引入稳定性能回归,也不足以宣称所有对照完全无退化。组合版本验证应保留 Q3 对照,不能把单补丁收益直接相加;不因此添加新的 merge blocker。

验证边界:当前 HEAD 的 CI 40034727403 已成功;diff --check 通过。本次未重跑重型 UT/TSBS,性能数字是对作者报告的复核而非独立复测。

当前认证账号也是 PR 作者,故以 COMMENT 记录建议通过;这不是 GitHub 正式 Approval。

This branch was successfully deployed

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

Labels

do-not-merge/wip kind/enhancement size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Performance]: Fixed MIN/MAX repeats provenance preflight for ordinary vectors

3 participants