Skip to content

Tests for the #155 encoding rewrites (carries the stack) - #290

Merged
jdatcmd merged 6 commits into
mainfrom
test/155-encoding-invariants
Jul 31, 2026
Merged

Tests for the #155 encoding rewrites (carries the stack)#290
jdatcmd merged 6 commits into
mainfrom
test/155-encoding-invariants

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@ChronicallyJD tests for #283, #284, #285 and #286, as asked.

I could not base this on perf/155-gorilla-batch because that branch lives on
your fork, so this branch carries your four commits unchanged plus one test
commit, and targets main. Merge it in place of #286, or cherry-pick the last
commit onto your stack, whichever you prefer. The four commits are untouched.

Five-major matrix with the stack plus this suite: ALL VERSIONS PASSED,
encode_invariants green on all five.

What it pins

PR check
#285 every bit-pack width the encoder selects, round-tripped against a heap mirror
#286 Gorilla across float patterns, through the batched writer
#284 dictionary at, below and above DICT_MAX_DISTINCT
#283 the premise the build skip rests on

Every check compares against a heap mirror rather than a literal, and each section
carries a control asserting the encoding under test was actually chosen.

The #283 check is the one worth your attention

Your claim is that skipping the build cannot change output. The step I could not
follow was "the dictionary is viable" implying "FSST is never selected", since the
gate is bestLen > rawLen - rawLen / 4.

That step is directly testable, so the suite tests it: no vector selects FSST at
or below the distinct cap
, across eight shapes on both sides of it, short and
long values. It holds, so the reasoning is sound.

The control comes first and matters more than the assertion: a high-cardinality
corpus that does select FSST, because "no FSST below the cap" is trivially true
on a suite that cannot observe FSST anywhere. My first pass at reviewing this had
exactly that flaw and I nearly reported a clean sweep as confirmation.

Two edges it does NOT cover, found by removal proofs failing to fail

This is the part worth checking, because both look covered and are not:

  • The (width == 64) mask. Reverted to the naive (1 << width) - 1, the
    suite stayed green. The encoder never selects packing at that width: measured,
    the dictionary wins above ~32 bits when distinct values are few, and when they
    are many across a wide range nothing wins, because packing near full width saves
    nothing over raw.
  • The ctz_in zero guard. Deleted, the suite stayed green. Gorilla encodes
    identical neighbours with a same-as-previous bit, so ctz_in is never reached
    with zero from SQL.

Both are correct to keep. Both are defensive code a suite cannot reach, and
covering them needs a C-level test. An earlier version of this file claimed to
cover the first and passed while the naive mask was in place, which is why the
comments now state the limit rather than the intent.

The packer check does have teeth: dropping the sub-byte offset from the field
span (nb) makes width 15 fail.

Still open from my review

Your ctz_in comment says x == 0 "keeps the old all-zeros answer"; the old loop
was undefined on zero rather than returning 0.

I have still not independently reproduced the performance numbers, and the offer
stands to measure them interleaved on the idle box the way #276 was done.

ChronicallyJD and others added 6 commits July 30, 2026 21:05
Profiling a text-heavy bulk load shows the load is CPU-bound on encoding, and the
single largest cost (~22%) is ColumnarFsstBuildChunkTable -- the FSST symbol-table
build. For a low-cardinality column the dictionary compresses far below the
threshold at which FSST is even attempted per vector, so the table is built and
then never used: the #276 cost-margin gates keep/drop but the expensive build runs
before that decision, unconditionally per text chunk.

Add ColumnarFsstDictWins: a cheap distinct-count over the corpus (bounded
open-addressing set, early-exit once the count exceeds DICT_MAX_DISTINCT). At or
below the cap the dictionary is viable for every 1024-row vector and wins, so the
FSST build is skipped; above it the column is a genuine FSST candidate and the
table is built as before. The probe reads the same corpus the keep/drop decision
uses and only skips when the dictionary wins for every vector, so stored bytes are
identical -- this trades wasted build CPU for nothing on disk.

Our own heuristic (FNV-1a bucketing + distinct count); FSST itself is the public
scheme (VLDB 2020).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T
encode_dict found each row's dictionary code by scanning every prior distinct
value with memcmp -- O(n * distinct) per vector. After the FSST-build skip routed
low-cardinality text to the dictionary, that linear search became ~10% of a text
load's CPU (the memcmp hotspot in profiling).

Replace it with an open-addressing hash of value -> distinct index (our own
FNV-1a + probe), making the lookup O(1) average. First-seen assignment order is
unchanged, so the dictionary and bit-packed codes are byte-identical; this is a
pure speedup.

Stacks on the FSST-build skip (#155). Our own code; no external source referenced.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T
bitpack() wrote each field one bit per loop iteration -- up to 64
branch-and-OR steps per value. Since every integer path (FOR, delta,
delta-of-delta, dictionary codes, FSST offsets) funnels through it, this
was the largest remaining self-cost in the bulk-load profile (~19%).

Mask each value to `width` bits, shift it into position, and emit the
<=9 bytes it spans directly. Each output byte is extracted with an
explicit shift, so the on-disk layout stays LSB-first and identical on
any host endianness -- the encoded bytes are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T
encode_gorilla was the last large pgColumnar self-cost in the bulk-load
profile (~10%). Two bit-at-a-time loops drove it:

- bw_put() appended one bit per iteration. Merge bits into a 64-bit
  accumulator and flush whole bytes at once. Same LSB-first stream, so
  the encoded bytes are unchanged.
- clz_in()/ctz_in() scanned for the leading/trailing zero bit one step
  at a time. Use pg_leftmost/rightmost_one_pos64() (portable hardware
  bit-scan) instead. Same counts.

No change to the on-disk format; output is byte-identical.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UX1jrWiQsJJA1t4pkmkb4T
…cover

Four PRs rewrote hot paths in the encoder and none of them was pinned by a test.
Three are rewrites that must emit the bytes the old code emitted; the fourth
changes a decision and claims the stored bytes follow.

A suite cannot compare against a build that no longer exists, so byte-identity is
not directly assertable. What it can do is pin the properties that make the claim
true and exercise the edges the rewrites introduced.

  1. Bit packing at every width the encoder actually selects, against a heap
     mirror. Proved by removal: dropping the sub-byte offset from the field span
     (nb) makes width 15 fail.

  2. Gorilla across float patterns, through the batched bit writer.

  3. Dictionary at, below and above DICT_MAX_DISTINCT, since the hash rewrite has
     to preserve first-seen order or the codes move.

  4. The premise the FSST build skip rests on, which is the one claim in the stack
     that is an argument rather than a mechanism: that no vector selects FSST while
     the distinct count is at or below the dictionary cap. If that holds, not
     building the table cannot change output. Asserted across eight shapes on both
     sides of the cap, with a high-cardinality control first, because "no FSST
     below the cap" is trivially true on a suite that cannot observe FSST at all.

Two edges are NOT covered, and the file says so rather than implying otherwise.
Both were found by removal proofs failing to fail:

  - The (width == 64) mask. Reverting it to the naive (1 << width) - 1 left the
     suite green, because the encoder never selects packing at that width:
     measured, the dictionary wins above ~32 bits with few distinct values, and
     with many distinct values across a wide range nothing wins, since packing
     near full width saves nothing over raw.
  - The ctz_in zero guard. Deleting it left the suite green, because Gorilla
     encodes identical neighbours with a same-as-previous bit and never reaches
     ctz_in with zero.

Both are correct to keep and both need a C-level test rather than a suite. An
earlier version of this file claimed to cover the first, and passed while the
defect it named was present.

Every check compares against a heap mirror rather than a literal, and each section
carries a control asserting the encoding under test was actually chosen.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The SQL suite could not reach two edges of the #155 rewrites, and both were found
by removal proofs that failed to fail: the (width == 64) mask in the bit packer,
because the encoder never selects packing at that width, and the zero guard in
ctz_in, because Gorilla encodes an unchanged value with a same-as-previous bit and
never XORs to zero.

columnar_debug_encoding_selftest calls the primitives directly and compares each
against a reference implementation of the algorithm it replaced. The references
are the pre-rewrite loops, kept deliberately naive because they are the oracle.
That checks the promise the rewrites actually made, which a suite cannot: not that
the data survives a round trip, but that the bytes are the ones the old code
produced.

  - bitpack at every width from 1 to 64, at nine value counts so the packing
    starts at every sub-byte offset, with random values plus the in-width extremes
  - bw_put across all widths in mixed order, so the carry between calls is
    exercised at each offset, comparing the byte stream and the residual carry
  - clz_in and ctz_in over four window sizes and the edges a hardware bit scan
    gets wrong, including zero, one, the top bit and all ones

Values come from splitmix64 with a fixed seed, so a failure is reproducible rather
than a story about a random run.

Both previously uncatchable defects are now caught:

  naive (1 << width) - 1 mask
    FAIL bitpack width=64 n=2: bytes differ from reference
  ctz_in zero guard removed
    FAIL and the self-test actually compared a meaningful number of cases: ran=none

The second is caught by the anti-vacuity control rather than by a comparison,
because pg_rightmost_one_pos64 asserts on zero and the backend dies before
returning a row. That is the control earning its place: without it a self-test
that produced nothing would have reported no failures.

utils/tuplestore.h is included explicitly because through PostgreSQL 18 it arrives
transitively behind funcapi.h and on 19 it does not. The five-major matrix caught
that as a build failure on 19 alone, which is the whole argument for running it.

Bound by the suite, not shipped in the catalog, like the other debug hooks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jdatcmd

jdatcmd commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Added the C-level half, so the two edges I reported as uncoverable are now
covered. Five-major matrix: ALL VERSIONS PASSED, encode_invariants green on
all five, now 11 checks.

columnar_debug_encoding_selftest calls the primitives directly and compares each
against a reference implementation of the algorithm it replaced. That checks
the promise these rewrites actually made, which a suite cannot: not that data
survives a round trip, but that the bytes are the ones the old code produced.

  • bitpack at every width 1 to 64, at nine value counts so packing starts at
    every sub-byte offset, random values plus the in-width extremes
  • bw_put across all widths in mixed order, so the carry between calls is
    exercised at each offset, comparing both the byte stream and the residual carry
  • clz_in and ctz_in over four window sizes and the edges a hardware bit scan
    gets wrong, including zero, one, the top bit and all ones

Values come from splitmix64 with a fixed seed, so a failure is reproducible rather
than a story about a random run.

Both previously uncatchable defects are now caught:

naive (1 << width) - 1 mask
  FAIL bitpack width=64 n=2: bytes differ from reference

ctz_in zero guard removed
  FAIL and the self-test actually compared a meaningful number of cases: ran=none

The second is caught by the anti-vacuity control rather than by a comparison:
pg_rightmost_one_pos64 asserts on zero, so the backend dies before returning a
row. Which means the guard is load-bearing in an assert build after all, not
merely defensive.

One finding from the matrix that is worth your attention beyond this PR.
utils/tuplestore.h arrives transitively behind funcapi.h through PostgreSQL 18
and does not on 19. The build failed on 19 alone, with 15 through 18 green. It
is fixed here with an explicit include, but it is the same class as the shims in
columnar_compat.h, and worth knowing if you add a set-returning function
anywhere else.

Also worth recording because it nearly cost the work: my first attempt at these
proofs destroyed the self-test. git checkout src/columnar_encoding.c restores
from the index, the new code was never staged, so the proof wiped it and I
committed a test that could not run. The matrix caught it as ran=none on all
five majors, which is the control doing exactly what it was written for.

@jdatcmd
jdatcmd merged commit 4cc95ce into main Jul 31, 2026
10 checks passed
ChronicallyJD pushed a commit that referenced this pull request Aug 2, 2026
The refresh in #320 rebuilt the open list from issue STATE and not from issue
CONTENT. #155 was still open, so its original text was carried forward verbatim,
including the "about 4.9x slower than heap" figure. That number was the problem
statement from before the work, not the result: four encoder levers (#283 to #286,
merged via #290) had already taken the 100M-row load from 783 s to 383 s and the
gap to heap from about 4.9x to about 2.2x, with a byte-identical on-disk image.

So the roadmap named finished work as the next thing to take, and quoted a measured
figure that had been superseded. #155 itself was left open only because the
resolution comment said "closing as resolved" without closing it; it is closed now,
after verifying the four commits are on main.

The entry becomes #300, which is the real remaining work: core COPY's per-field
parse, paid identically by heap and TimescaleDB, and not reachable by another
encoder change.

Refs #155, #300. No issue is closed by this commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011miCFRSatixeNRw3w5yNq8
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.

2 participants