fix: GROUP BY sort record carries only needed columns, not full row - #670
Merged
Conversation
…665) #506 already skipped the real per-row Column/Rowid read for a column compile_grouped_scan's needed-columns analysis found unused, but kept the sort record itself full-width (a cheap Null placeholder in place of each skipped column) — every downstream reader of the pass-2 pseudo cursor assumed a 1:1 position match with schema's original column layout, so truly narrowing the record would have broken that mapping. This closes that gap: pass 1's MakeRecord/SorterInsert now includes only needed_order's columns (no Null padding), pass 2 resolves GROUP BY keys and aggregate arguments against a compacted synthetic TableSchema (so name-based Scope/compile_value resolution translates automatically), and the couple of by-index reads (the GROUP BY key's OrderByTarget::Column case, and the arbitrary-row snapshot loop) translate through an explicit original-index -> compacted-position map. flush_group's own synthetic schema/snapshot_regs stay full-width and untouched — that's a separate, per-group (not per-row) pseudo cursor unaffected by this ticket. hash.rs's unwired #631 spike keeps its own pre-#665 copy of the old Null-padded helper, since threading the same compaction through a dead code path isn't worth it. Verified against oracle: `read_group_by_agg`'s sorter MakeRecord is now 2-wide (bucket, x), byte-for-byte matching sqlite3's own EXPLAIN. Full test suite, corpus (380 tests), and parity suites all pass unchanged. Closes #665 spend: ~1x estimate (more involved than the issue's "small" estimate — true record narrowing required a compacted synthetic schema/index- translation layer, not just skipping the analysis #506 already had)
iheitlager
force-pushed
the
fix/665-group-by-sort-record-narrowing
branch
from
August 30, 2026 12:15
e84ee35 to
22c1750
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
compile_grouped_scan's pass 1 (src/codegen/select/aggregate.rs) built a full-width sort record per input row (one register/column perschemacolumn) even after fix: GROUP BY aggregate (unindexed) is ~11x slower than SQLite oracle due to full-row column overhead in sort pipeline #506 taughtcolumns_needed_for_projectionwhich columns were actually needed — fix: GROUP BY aggregate (unindexed) is ~11x slower than SQLite oracle due to full-row column overhead in sort pipeline #506 only skipped the realColumn/Rowidread for an unneeded column, substituting a cheapNullplaceholder, because every downstream reader of the pass-2 pseudo cursor assumed the record still mirroredschema's original column layout 1:1.compile_row_values_compactnow emits onlyneeded_order's columns (noNullpadding at all), soMakeRecord/SorterInsertgenuinely narrows. Pass 2's GROUP BY key reads, aggregate-argument compilation, and the plain-column "arbitrary row" snapshot all resolve against acompact_schema(a syntheticTableSchemacovering only the needed columns, in order) — name-basedScope/compile_valueresolution translates automatically, and the couple of by-index reads translate through an explicitcompact_index_map(original schema index -> compacted position).flush_group's own synthetic schema/snapshot_regsstay full-width and untouched on purpose — that's a separate, per-group (not per-row) pseudo cursor, unaffected by this ticket's scope.hash.rs's unwired perf: implement Sorter opcodes for GROUP BY (4× gap vs oracle) #631 spike (dead code, kept for possible reuse) keeps its own pre-perf: GROUP BY/aggregate sorter records are full-row-width instead of narrowed to needed columns #665 copy of the old Null-padded helper rather than threading the same compaction through a path that's never actually invoked.Verification
SELECT bucket, COUNT(*), SUM(x) FROM bench_data GROUP BY bucket's sorterMakeRecordis nowMakeRecord|0|2(bucket, x) — byte-for-byte matching sqlite3 3.53.4's ownEXPLAINoutput for the same query (alsomkrec(r[11..12]), 2-wide), where it was previously 6-wide with 4Nullplaceholders.Test plan
group_by_excludes_unreferenced_columns_from_the_sort_record— a 4-column fixture with one column referenced nowhere, asserting the sort record'sMakeRecordwidth is exactly 3 (GROUP BY key + plain arbitrary-row column + aggregate argument, excluding the unreferenced column), plus correctness against the real oracle.cargo test --release— full suite green (980 tests, 0 failed).make test-corpus— 380 tests, byte-exact against the real oracle, all pass.make test-parity— 19 pass, 5 skipped (unimplemented V-blocks), 0 failed.cargo clippy --all-targets/cargo fmt --check— clean.cargo bench --bench crud -- read_group_by_agg— ~3.3% faster on the standard 6-column bench fixture (modest, since the fixture isn't very wide — the correctness win, matching oracle's opcode shape exactly, is the more solid deliverable; the remaining ~2.7x gap vs oracle on this scenario has a different root cause outside this ticket's scope).Closes #665
spend: ~1x estimate — more involved than the issue's "small" estimate, since true record narrowing required a compacted synthetic schema/index-translation layer rather than just reusing #506's existing needed-columns analysis.