Skip to content

perf(sql): reuse join scratch row instead of NxM clones (SQLR-4) - #174

Merged
joaoh82 merged 1 commit into
mainfrom
feat/sqlr-4-join-scratch-alloc
Sep 19, 2026
Merged

joaoh82 merged 1 commit into
mainfrom
feat/sqlr-4-join-scratch-alloc

Conversation

@arthur-dent-agent

Copy link
Copy Markdown
Collaborator

Summary

  • Reuse a single scratch Vec<Option<i64>> in execute_select_rows_joined instead of cloning the left row for every right candidate.
  • Non-matching pairs no longer heap-allocate; matches still clone into the accumulator.
  • Scratch capacity is the full join width; len stays the in-scope width so later tables are not visible to JoinedScope.
  • Nested-loop O(N×M) bound is unchanged (SQLR-4 / MARVIN SQLR-4).

Test Plan

  • cargo test --lib join — 39 passed (INNER / LEFT / RIGHT / FULL / CROSS / NATURAL / USING / chained / aggregates)
  • Human: optional make bench W9 (inner-join) before/after if you want a number; not run here (criterion, local-only)

Closes SQLR-4

execute_select_rows_joined cloned the left row for every right
candidate. Reuse one Vec per join fold and clone only on match.
Behavior is unchanged; existing join unit tests cover the flavors.
@vercel

vercel Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
rust-sqlite Ready Ready Preview Sep 19, 2026 12:32am UTC

Request Review

@marvin-agent-rockflow marvin-agent-rockflow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes Agent Review

Head: c559d05 · Files: 2 · +21 / -9
Verdict: Approve

Critical

  • None

Warnings

  • None

Suggestions

  • Nit only: scratch is created inside each join fold, so Vec::with_capacity(joined_tables.len()) does not actually retain capacity across later folds (the comment implies it does). Harmless over-alloc on early folds; lift the buffer outside the for join loop if you want the comment to match.

Looks good

  • Non-matching (left, right) pairs no longer heap-allocate; matches still clone into next_acc.
  • LEFT/FULL unmatched padding still uses left_row + None, not the scratch trailing slot (which would still hold the last right rowid after the inner loop). RIGHT/FULL unmatched-right emission is unchanged.
  • JoinedScope borrows scratch only for eval_predicate_scope; no aliasing across scratch[right_pos] overwrites.
  • In-scope len (right_pos + 1) vs full-join capacity is the right split so later tables stay invisible to ON evaluation.
  • Docs sentence matches the change; nested-loop O(N×M) bound is unchanged as advertised.
  • CI green (rust / lint / SDKs / coverage).

Automated hourly review by marvin-agent-rockflow (Hermes). Will re-review only if new commits land.

@arthur-dent-agent arthur-dent-agent 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.

Hermes Agent Review

Head: c559d05 · Files: 2 · +21 / -9
Verdict: Comment

Own PR (arthur-dent-agent) — COMMENT only. This HEAD would otherwise Approve.

Critical

  • None

Warnings

  • None

Suggestions

  • LEFT/FULL unmatched padding still allocates a new padded from left_row rather than resetting scratch[right_pos] = None. That is the safer choice (scratch still holds the last right rowid) and not worth changing.
  • Nested-loop O(N×M) is unchanged, as documented. Optional W9 bench remains local-only; existing cargo test --lib join (39) is the right gate.

Looks good

  • One scratch Vec<Option<i64>> per join fold; clear + left prefix + overwrite scratch[right_pos] so non-matches do not heap-allocate.
  • Capacity is full join width; len stays right_pos + 1 so JoinedScope does not see later tables.
  • Matches still clone into next_acc before the next right candidate mutates scratch.
  • LEFT/FULL unmatched path uses left_row (not the dirty scratch). RIGHT/FULL unmatched emission is unchanged.
  • Docs in sql-engine.md match the code.

Automated hourly review by arthur-dent-agent (Hermes). Will re-review only if new commits land.

@joaoh82
joaoh82 merged commit fc5bbad into main Sep 19, 2026
21 checks passed

This branch was successfully deployed

1 active deployment
Preview — c559d054 Deployed Sep 19, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants