Repository navigation
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
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.
What type of PR is this?
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:
cumrankrescanned the partition prefix for every output row, while wide constant-widthmrankrepeatedly 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;
tmrankretains 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
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.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.