Skip to content

Improve local rank performance and clean up follow-up debt - #31516

Merged
XuPeng-SH merged 4 commits into
matrixorigin:mainfrom
XuPeng-SH:fix/engineering-quality-followup-20261028
Oct 28, 2026
Merged

XuPeng-SH merged 4 commits into
matrixorigin:mainfrom
XuPeng-SH:fix/engineering-quality-followup-20261028

Conversation

@XuPeng-SH

@XuPeng-SH XuPeng-SH commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • Improvement
  • Code Refactoring
  • Test and CI
  • Documentation

Which issue(s) this PR fixes:

Follow-up to #29947 and the changes in #31020, #28987, #31000, #30054 and #31262.

What this PR does / why we need it:

cumrank rescanned the partition prefix for every output row, while wide constant-width mrank repeatedly scanned its frame. Use a compressed frequency index that starts at the current output demand and grows geometrically. This reduces repeated comparisons without indexing the entire partition before the first result. Rebuilds seed the active physical frame, preserving NULL, NaN, tie and eviction semantics.

The index is optional and accounted. Capacity rejection falls back to scanning; if required output allocation runs out of capacity, release the index and retry the same fixed-width append once. Cancellation and invalid-account errors still propagate. Completed partitions, reset and free release the index. Small/variable widths and all-NULL partitions retain scan paths; tmrank retains its civil-time semantics.

Also share the SORT ROLLUP type capability between planner and executor, consolidate Python identifier quoting while preserving API behavior, fix test-helper cleanup on error, correct the Decimal64 DISTINCT benchmark's group distribution and repeated-hit workload, and replace review-history transcripts with links. Existing inexpensive small-input oracle tests remain.

Design and execution/memory contract at this revision.

Validation

  • Full Window package tests and full package race run passed. Regressions cover output-budget fallback, later-batch pressure, failed retry, cancellation, sealed accounts, reset/reuse, short/NULL prefixes and frame seeding across growth.
  • Focused ROLLUP planner/executor, Decimal64 and nonempty colexec mock lifecycle race checks passed; unchanged-path evidence remains applicable.
  • Incremental go vet, configured golangci-lint v2.14.0 and molint passed for affected Go packages. Golangci reported no new issues; molint emitted only pre-existing diagnostics on unchanged source.
  • Python SQL parameter/identifier boundary suite: 105 passed.
  • Isolated single-CN SQL smoke through the real MySQL frontend: 8,205 rows with NULLs and ties; cumulative and width-128 ranks matched an independent row-by-row oracle across output-batch growth.
  • Same-host baseline/candidate microbenchmarks used the same harness and native build, GOMAXPROCS=2, three one-operation samples. Medians against the merge-base implementation:
Workload Baseline This PR
1M rows, mrank width 128, first output 17.64 ms 4.56 ms
1M rows, mrank width 128, full consumption 2,174 ms 1,329 ms
1M rows, cumrank, first output 505.95 ms 4.41 ms
16K rows, cumrank, full consumption 2,069.90 ms 7.91 ms

Width-127 scan control remained approximately 32 ms. These are microbenchmarks, not production workload results. Geometric rebuilding raises total allocated bytes for full 1M-row width-128 consumption from about 41.7 MB to 91.9 MB; first-output allocation is about 8.86 MB versus 8.65 MB. These figures are cumulative allocation, not peak memory. Full 100K-row all-NULL cases were approximately 0.6–0.9 ms slower.

Input remains materialized. Variable-width/time-based rank scans, explicit-frame weighted percentile, and ROLLUP state reuse remain separate follow-ups. Distributed BVT was not run locally; CI for the updated head is pending.

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

@XuPeng-SH

XuPeng-SH commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

mo-bot review · mo-bot-local · done

Commit: f014fd507506e0dc96f6bbe9565c1042c6903a07

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

mo-bot review · mo-bot-local · review

Commit: f014fd507506e0dc96f6bbe9565c1042c6903a07

Assessment: request_changes

Reviewed the complete delta. Correctness and lifecycle checks passed, but eager indexing introduces a substantial first-output performance regression.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1

  • P2 pkg/sql/colexec/window/local_rank_index.go:75: Constant-width mrank(v,128) over a large partition with an early-stopping consumer, such as LIMIT 1.
    Impact: The first output now requires sorting and indexing the entire partition. For 1,048,576 rows, measured first-batch time increased from 17.92 ms to 852.15 ms (47.55x), with approximately 24 MiB additional allocation. Defer index construction or retain scanning when early consumption cannot amortize that work.
    Evidence: Three same-input samples per version used the frozen base window.go as an overlay and identical native libraries. The SQL planner probe confirms LIMIT remains above Window. The design acknowledges possible first-output overhead, but the reported cumulative-rank sample does not cover this large-partition, fixed-width regression. Retained evidence: attempt-1/benchmark-summary.json, baseline-bench.log, candidate-bench.log and plan-probe.log; receipts 1793161727359569792-3.json, 1793161686660684052-3.json and 1793161908173242754-5.json under checks/.

Validation:

  • Full Window package passed normally and under race detection; focused cancellation, generation and early-reset/reuse stress checks passed.

  • Selected ROLLUP planner/executor, mock lifecycle and Decimal64 tests passed; all 105 Python boundary tests passed.

  • Benchmarks reproduced the intended full-consumption gains and a stable width-127 control.

  • Native provenance verified. Command/source/exit receipts and logs reconciled in validation-reconciliation.json; complete discussion dispositions saved in discussion-assessment.json.

Limitations:

  • Performance measurements cover operator execution, not end-to-end SQL latency; SQL reachability was checked through the parser/planner and compiler ownership chain.

  • No live-server or distributed BVT run; frozen CI remained pending. No substantive prior review findings or author rebuttals were supplied.

@XuPeng-SH

XuPeng-SH commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

mo-bot review · mo-bot-local · done

Commit: e4fb331b5229b52ef8613151e083c8e7dbe1966c

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

mo-bot review · mo-bot-local · review

Commit: e4fb331b5229b52ef8613151e083c8e7dbe1966c

Assessment: approve

The demand-driven index resolves the prior first-output regression while preserving rank semantics and allocation fallback. No actionable findings remain across the complete delta.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

Validation:

  • Full Window package passed normally and under race detection, including growth, frame seeding, cancellation, output pressure and reset/reuse tests. Focused vet, formatting and diff checks passed.

  • Nineteen baseline/candidate benchmark cases completed three samples each. For 1M rows at width 128, first-output median improved from 21.42 ms to 4.29 ms; full consumption improved from 2.43 s to 1.38 s.

  • Independently verified and reused unchanged Python boundary tests (105 cases), focused ROLLUP/Decimal/mock tests and SQL-planner LIMIT-placement evidence.

  • Native provenance, nine terminal receipts and retained logs were reconciled in attempt-1/validation-manifest.json. Complete discussion dispositions are saved in discussion-assessment.json.

Limitations:

  • Full-consumption allocation increased from approximately 41.7 MB to 91.9 MB, an accepted documented tradeoff protected by accounting and capacity fallback.

  • No new live-server or distributed BVT run. Benchmarks measure operator execution and cumulative allocation, not production latency or peak memory.

This branch had an error being deployed

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

Labels

kind/documentation Improvements or additions to documentation kind/enhancement kind/refactor Code refactor kind/test-ci size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants