Skip to content

perf(query): keep the keyed-upsert key map on the table instead of rebuilding it - #512

Merged
singaraiona merged 27 commits into
devfrom
perf/upsert-key-index
Sep 14, 2026
Merged

singaraiona merged 27 commits into
devfrom
perf/upsert-key-index

Conversation

@ser-vasilich

@ser-vasilich ser-vasilich commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Keyed upsert rebuilt its key→row map on every call, which made a sequence of
batches into one growing table quadratic in the table size. Reported in #511.

The defect

upsert_apply built the map over every existing row on entry and freed it
on exit:

if (!err && m > 1) {
    upsert_map_init(&map, nrows0 + m);
    for (int64_t r = 0; r < nrows0 && !err; r++)          // every row already there
        upsert_map_put(&map, upsert_hash_row(slots, kci, nk, r), r);
}
...
if (map.hdr) scratch_free(map.hdr);                        // and discarded

So K batches cost O(K * rows). upsert_hash_row has exactly one call site —
that loop — which is why a profile of a filling table is dominated by it and by
upsert_map_put while the probe side (upsert_hash_atoms / upsert_map_find)
barely registers: nearly all the work hashes rows that were already in the
table.

m == 1 had no map at all and scanned the table linearly for its one key.

The fix

The hot shape already requires a named table mutated in place, so the table
object survives between calls and the map can live on it.

RAY_IDX_UKEY is a new index kind holding the open-addressing slot array, the
key column positions and the row count it describes. It attaches to the
RAY_TABLE rather than to a column because the key spans several of them, and
rides the type-agnostic RAY_ATTR_HAS_INDEX arms of
ray_retain_owned_refs / ray_release_owned_refs, so it dies with the table.

It cannot reach a file: serde masks attrs down to HAS_NULLS and col_save
strips HAS_INDEX. The union arm is sized to the existing hash arm (a static
assert pins this) so ray_index_t does not grow — it is stored inline in
persisted column files and a larger payload would move that on-disk layout for
every kind.

Maintenance is what upsert already did: a matched row updates non-key columns
only, so the mapping is untouched, and an appended row is one insert the append
path was already making. Capacity grows by doubling, so rebuilds amortize.

Created only on upsert, for any key width 1..8, when the table is a named
in-place binding with no index yet. Anything else starts cold and pays the old
price once: a copied table, a wider key, a map that could not be allocated.

The second commit lets a single-row upsert use the same map. The scan it saves
is the smaller half — the point is that a row appended outside the map moves
the table's row count, the next batch then finds the map stale and rebuilds
over everything. One interleaved single-row upsert per batch put the entire
quadratic back.

Reproducer

(set levels (table [instrument side px sz]
  (list (take ['x] 0) (take ['x] 0) (take [0] 0) (take [0] 0))))
(set B 500)
(map (fn [i] (upsert 'levels 3 (table [instrument side px sz]
  (list (take ['x] B) (take ['b] B) (+ (til B) (* i B)) (take [7] B))))) (til NB))
(println (count levels))

Release build, -c 1, same host:

rows before after
50 000 84 ms 38 ms
200 000 1042 ms 119 ms
400 000 5562 ms 238 ms
800 000 34119 ms 543 ms

The after column doubles when the work doubles; the before column does not.

Replacing the body of the loop with a batch followed by one single-row upsert
(the shape a snapshot-then-updates feed produces):

rows before after
50 000 54 ms 30 ms
100 000 209 ms 42 ms
200 000 719 ms 77 ms

Single-row upserts only, into a table growing to N rows:

calls before after
8 000 33 ms 32 ms
16 000 73 ms 50 ms
32 000 172 ms 85 ms

The 8 000 row is the cost of the change: building the map is slightly dearer
than the scan it replaces, and it pays for itself by 16 000.

On the shape from #511 (245 313 rows, 494 batches, (sym, sym, i64) key,
245 073 final levels), the reporter's own growth metric — the sum of the first
50 batch times against the last 50 — goes from 15.4x to 1.5x, and the
replay from 1666 ms to 256 ms.

Interleaving another in-place mutation between batches used to restore the
whole quadratic, because each one left the map no longer describing the table
and the next batch rebuilt it over every row. The third commit keeps the map
across both:

200 000 rows, 500-row batches, plus one of these after each batch before after
single-row upsert 719 ms 77 ms
insert 708 ms 74 ms
update of a non-key column 801 ms 152 ms

insert appends in ascending row order, so entering its rows preserves the
map's "duplicates answer lowest row first" invariant. The where-update path
now drops the map only when the update writes a key column — the one mutation
the recorded row count cannot see, since a cell rewrite leaves the count
alone.

Why a stale map cannot corrupt data

upsert_map_find never decides on its own: every candidate slot is verified
against the actual key cells with upsert_row_eq_atoms before it is accepted.
The map is an accelerator, not an authority, so a stale or colliding entry
cannot write into the wrong row. The worst it can do is miss a match and
append a duplicate key — which is the symptom every test below asserts on.

mutation caught by
row count moved, either direction recorded count in ukey_fits → rebuild
count unchanged, key cell rewritten update_where_inplace drops when a key column is written; alter's set path already calls ray_index_drop, which works on a table
table copied fresh object starts with no map
error mid-batch appends rolled back, map dropped

Scope

Does not address the deletion-by-key-prefix request in the same issue; that is
a separate feature, so this does not close #511.

Tests

test/rfl/table/upsert_inplace.rfl covers reuse across batches, a key cell
rewritten in place by a where-update, an insert between upserts, a key of a
different width on the same table, a refused batch, and an interleaved
single-row upsert.

Each guard was removed in turn to confirm the tests are not vacuous:

  • dropping the map in update_where_inplacecount u is 8, not 7 (duplicate key)
  • the recorded row-count check → count n is 7, not 6
  • map maintenance on a single-row append → [1 2 3 4 4 5], not [1 2 3 4 5]

An earlier version of these tests passed either way: every upsert in it carried
one row, and a one-row upsert built no map at all.

3770/3770 under ASan, with workers and serially.

ser-vasilich and others added 4 commits September 11, 2026 12:23
…building it

upsert_apply built its key->row map over every existing row on entry and
freed it on exit, so a sequence of bulk batches into one growing table cost
O(batches * rows) — the map rebuild, not the batch, dominated. A profile of a
filling table is almost entirely upsert_hash_row and upsert_map_put, both of
which only ever touch rows that were already there; the probe side of the
batch barely registers. The per-batch cost grew with the table.

The hot shape already requires a named table mutated in place, which means
the table object itself survives between calls, so the map can live on it.
RAY_IDX_UKEY is a new index kind holding the open-addressing slot array, the
key column positions and the row count it describes. It attaches to the
RAY_TABLE rather than to a column because the key spans several of them, and
rides the type-agnostic HAS_INDEX arms of retain/release, so it dies with the
table. It is runtime-only: serde masks attrs down to HAS_NULLS and col_save
strips HAS_INDEX, so it cannot reach a file. The arm is sized to the existing
hash arm so ray_index_t does not grow, which would have moved the on-disk
layout of persisted column indexes.

Maintenance is what upsert already does: a matched row updates non-key
columns only, so the mapping is untouched, and an appended row is one insert
the append path was already making. Capacity grows by doubling, so rebuilds
are amortized. Everything else starts cold and pays the old price once: a
table that is copied, a key wider than the arm holds, a map that could not be
allocated.

Staleness is caught by the recorded row count, and the in-place where-update
path drops the map explicitly — it can rewrite a key cell without moving the
row count, which is the one mutation the count cannot see. The tests cover
both, and were checked by removing each guard in turn.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… map

The map was built for batches only, so a single row still scanned the table
for its one key. The scan was the smaller half of the problem: the row it
appends moves the table's row count without the map learning about it, so the
next batch finds the map stale and rebuilds it over every row. One interleaved
single-row upsert per batch was enough to put the whole quadratic back, and a
feed of snapshots followed by individual updates is exactly that shape.

A single row now takes the same path when the table carries a map: it probes
instead of scanning, and its append enters the map like a batch's does. It
still never builds one from nothing — without a table to hang it on a map
costs the scan it would replace and is then discarded, so the scratch map
stays gated to batches.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more paths mutate a named table in place, and both sent the next batch
back to rebuilding the map over every row — which is the cost the map exists
to avoid. Interleaving either one per batch restored the whole quadratic.

insert appends without consulting keys, so it moved the row count while the
map learned nothing; the map then no longer described the table and the next
upsert rebuilt it. Entering the appended rows costs what the append already
cost, and insert appends in ascending row order, so the map's "duplicates
answer lowest row first" invariant survives. A map that cannot take them is
dropped rather than left naming a shorter table.

The where-update path dropped the map unconditionally. Only a write into a
key column can invalidate it — and that is the one mutation the recorded row
count cannot see, since a cell rewrite leaves the count alone. Any other
column leaves the mapping exactly as it was, so the drop is now conditional
on the update's target columns, which that path already resolves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ray_alloc_copy memcpys the whole 32-byte header, so a copied table carried the
RAY_IDX_UKEY pointer and ray_retain_owned_refs took a reference to it. The map
records the row count it describes and upsert mutates it in place, so two
tables sharing one would each write their own count into it. The recorded
count makes that safe today — the second table finds the map stale and rebuilds
— but the map is meant to belong to one table, and ray_table_add_col copies a
table whenever a column is added. A copy now starts without it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ser-vasilich and others added 7 commits September 12, 2026 12:44
alter set retained its target before handing it to ray_cow, so ray_cow's own
`rc == 1` test could never be true and the copy path fired on every call. A
one-element write copied the whole vector: the cost of an alter scaled with
the vector's length rather than with the write, and 200 single-element writes
into a 4M vector spent half a second copying 32 MB at a time.

The retain is not the bug — it is there because ray_cow releases its input,
and handing it a borrow would decrement the binding's own reference. What was
missing is that sole ownership after that retain is `rc == 2`, the binding's
reference and alter's, which is the condition upsert's in-place path already
uses. Test for it directly and skip the copy.

The three exclusions keep the previous behaviour exactly: ray_cow returns an
arena block as-is, and a slice or a mapped block is not ours to write through,
so all three still take the old path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Groundwork for an in-place delete, which has to vacate the entries of the
rows it removes. Clearing a slot is not an option: the map is open addressing
with linear probing, so a hole in the middle of a chain ends the walk early
and orphans every entry displaced past it. A vacated entry becomes
UKEY_SLOT_TOMB and probing walks over it.

put keeps taking the first genuinely empty slot rather than reusing a
tombstone. Reuse would place a later row ahead of an earlier one in its
chain, which changes which of two duplicate keys a keyed upsert updates —
the map's one behavioural promise beyond finding a match at all.

Tombstones are counted because they hold a slot without being a row: a load
test that ignored them would let the array fill completely, and the free-slot
scan in put would then not terminate. kci narrows to int16 to keep the arm
inside the hash arm's 64 bytes, which the static assert pins — ray_index_t is
stored inline in persisted column files and must not grow.

Nothing creates a tombstone yet, so this changes no behaviour.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing rows meant `(set t (select {from: t where: <negated>}))`, which binds
a new table — and a fresh table starts without the upsert key map, so the next
batch rebuilt it over every row. On a 245k-row book the filter was a fifth of
a reset's cost and re-earning the map was the rest.

`(delete {from: 't where: P})` compacts the named table in place and patches
the map instead of discarding it. `(delete {from: t where: P})` answers with a
new table. Both evaluate P once and compact the same matched-row list with the
same code, so they cannot disagree about which rows go; the functional form is
deliberately not `select` over a negated predicate, which would be a second
expression to keep in step.

Compaction is a memmove of the runs between deleted rows. Every column type
stores a fixed-width element — a RAY_STR cell is a 16-byte descriptor and the
pool it points into does not move with the row — so no column needs a
per-cell path, and a boxed column only releases the cells it drops first. Row
order survives, so a sorted column stays sorted; an attached accelerator index
does not, and is dropped.

The map is patched, not rebuilt: compaction moves a surviving row from r to
r - (deletions before r) and changes no key, so every entry keeps its bucket
and only the row number it stores shifts. Entries naming a deleted row become
tombstones. An old-to-new row map is built in one pass so each slot costs O(1)
— asking a binary search per slot instead cost log(k) on every slot of the
capacity and dominated the patch. A large deletion is handed to the next
upsert instead, which rebuilds and reclaims the tombstones.

`where:` is required: a delete without one would empty the table, which is too
easy to reach by leaving a clause out.

The in-place gate is read before the predicate runs, because evaluating it
binds the table's columns into a query scope and the reference counts are only
quiet enough to test beforehand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ser-vasilich and others added 16 commits September 13, 2026 17:20
The in-place test reads the reference count of what ray_env_get returned, and
for a dotted name that is the leaf living inside the container — the vector in
a dict's vals list, or a table's column. That leaf usually holds the only
reference to itself, so it passes an rc test that says nothing about whether
the CONTAINER is shared:

  (set d (dict ['x] (list (til 3))))
  (set e d)
  (alter 'd.x set 0 99)
  e.x        ;; [0 1 2] before, [99 1 2] after

env_set_dotted COW-rebuilds the dict chain afterwards, but it cannot undo a
write that already landed in the shared leaf. The copy is what made this safe.
A table head is the same resolution and changed behaviour too: env_set_dotted
refuses it, so the amend used to be discarded, and in place it reached the
live column of the table and of every alias.

Exclude a dotted name from the fast path. The tests covered flat names only,
which is why nothing caught it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e misses

`from: 't` promises the binding is amended and the symbol comes back. The gate
can miss for reasons the caller cannot see — a column at rc 2 because a
literal in the parse tree holds it, a second binding on the table, an mmapped
or narrow-SYM column — and the copying form then returned the new table
without rebinding, so the binding kept all its rows and the caller got a value
it did not ask for. Every sibling verb rebinds on this path.

Two shapes the copying form also had no backstop for. A parted table's columns
carry the RAY_PARTED_BASE wrapper, which describes no flat element the
compaction could move; flatten once at entry, as ray_update does. A slice
column is a header-only view whose ray_data resolves into the parent's
storage, so compacting the copy would move the PARENT's elements — the
in-place path declines one through upsert_col_ok, and the copying form now
refuses rather than corrupt a buffer somebody else owns.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**A key wider than the map's arm left a stale map attached.** Above
RAY_UKEY_MAX_COLS the whole ukey block was skipped, so an already attached map
was neither refreshed nor dropped. The call still writes every non-key column
of a matched row in place, and those may be the attached map's key columns;
with no row appended the row count does not move either, so ukey_fits accepted
the map as fresh on the next call and a keyed upsert appended a duplicate key.
Drop the map instead of skipping.

**`.idx.drop` on a table carrying the map dereferenced NULL.** ray_index_drop
COWs its target first, and ray_alloc_copy deliberately does not carry a
table's key map to the copy — so the copy has no index left to detach, and
reading it anyway crashed. Before the map a table never carried HAS_INDEX and
this was a no-op; return early when the index is already gone.

**`.mem.objsize` reported a mapped table as nothing but its map.** The
HAS_INDEX arm returns before the TABLE arm, which was harmless while only
vectors could carry an index. A 20k-row table measured 320 KB without the map
and 524 KB with it — the map alone, both columns skipped. Fall through for a
table.

Each fix has a regression case, and each was checked by putting the fault back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The footprint accounting added to dev walks an index's child blocks through a
fifth switch over ray_idx_kind_t, which this branch's new kind did not answer.
-Werror=switch caught it in the PR's merge-result build; a local build of the
branch alone does not see the new function at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fix that clears it in ray_alloc_copy had no case of its own. What is
observable is that a second binding keeps its own rows: the upsert declines
the in-place path at rc 3 and works on a private copy, which must not inherit
a map describing rows it does not have.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The flatten-at-entry arm had no case. Both forms are pinned, including that
`from: 'p` rebinds the name to the flattened table — a behaviour change worth
having written down rather than discovered.

The slice guard next to it stays uncovered and says so: no producer reachable
from the language puts a slice into a table column, so it is defensive only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat(query): delete, an in-place row removal that keeps the key map
perf(alter): amend a sole-owned value in place instead of copying it
@singaraiona
singaraiona merged commit 29e2476 into dev Sep 14, 2026
9 checks passed
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.

Optimize bulk keyed upsert for compound fixed-width keys

2 participants