Skip to content

fix(in): admit the typed kernel for text columns, which it never reached - #607

Open
singaraiona wants to merge 2 commits into
devfrom
fix/in-text-null-gate-593
Open

singaraiona wants to merge 2 commits into
devfrom
fix/in-text-null-gate-593

Conversation

@singaraiona

@singaraiona singaraiona commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

What & why

in gated its typed-kernel fast path on ray_vec_may_have_nulls for both
operands. That predicate returns true unconditionally for SYM and STR:

static inline bool ray_vec_may_have_nulls(const ray_t* v) {
    if (!v) return false;
    if (v->type == RAY_SYM || v->type == RAY_STR) return true;

Their null is a payload value (id 0 / ""), not an attribute bit, so attrs
cannot prove a text column null-free. The gate was therefore never satisfiable
with a text operand, and every in over a symbol or string column fell
through to the generic per-row hashset probe.

The kernel it could not reach carries a verdict-LUT written specifically for
SYM — one byte load per row regardless of set size. It was dead code for the
only type that path was built for.

vec.h states the distinction where it defines the two predicates:
may_have_nulls is "a row-kernel gate, not a reason to reject a text
optimization"
, and "paths requiring null-free data use has_nulls below".
This is an admission check, so it now asks ray_vec_has_nulls, which inspects
the payload for text and is documented as being kept off per-row paths.

Measurements

351,393-row SYM column, 766 distinct values, -c 2:

needles before after
1 4,820 us 200 us 24x
2 5,180 us 160 us 32x
10 8,320 us 160 us 52x
100 8,660 us 180 us 48x
766 8,440 us 180 us 47x
50,000 7,360 us 520 us 14x

The symptom reported in #593 was the degenerate case: a one-element right side
costing ~120x the equivalent (== col x). That gap is now ~5x. But this is not
a small-needle special case — it lands on every text in, which is why the
whole column moves.

ClickBench (10M rows, 43 queries × 3, splayed): sum of per-query hot times
5690.7 ms → 5664.6 ms, no query beyond +4%. The exact check costs one pass over
each operand and does not show.

Correctness

Null semantics are the risk the old gate existed to manage: the kernel treats a
null as matching nothing, while this path is null-equals-null. Admitting only
exactly null-free operands keeps the two from being conflated — a null on
either side still falls through.

All eight null shapes were diffed against the pre-fix binary and are identical,
including the asymmetric case where only the needles carry a null. They are
now pinned in test/rfl/collection/in.rfl, which had no null coverage at
all
— which is how this stayed invisible. Added alongside them: the null-free
shapes that now reach the kernel, the degenerate single-needle case asserted
against the equality it should match, and duplicate needles.

This site was the only one

An earlier draft of this PR claimed other admission sites gate on
may_have_nulls the same way and were "likely the same bug". I checked them
rather than leaving the claim standing, and it was wrong — every other reader of
that predicate is correct:

  • ray_sorted_range_rowsel (idxop.c) does gate on may_have_nulls, but four
    lines below it accepts only float and integer/temporal column types, so text
    never reaches the path regardless.
  • str_scalar_int (string.c) gates the same way, and its switch accepts no text
    type — text returns false through default.
  • fills_vec_eager (collection.c) names RAY_SYM and RAY_STR explicitly in
    the same condition and returns them unchanged by design.
  • The remainder read the predicate in the direction where an always-true answer
    is the safe one (propagating HAS_NULLS, or an early return that skips work).

So this is a one-off at a single call site, not a systemic pattern. Recording
the correction because the original claim was in the commit message too, and the
commit has been amended.

Refs #593 (fixes its request 2 as a side effect; request 3, compound
membership, remains and is tracked in #596).

Checklist

  • PR targets dev (not master)
  • Commits follow Conventional Commits (feat: / fix: / perf: / docs: / …)
  • make builds cleanly (no new warnings)
  • make test passes; tests added/updated for behaviour changes

`in` gated its typed-kernel fast path on ray_vec_may_have_nulls for both
operands.  That predicate returns TRUE unconditionally for SYM and STR —
their null is a payload value (id 0 / "") rather than an attribute bit, so
attrs cannot prove a text column null-free.  The gate was therefore never
satisfiable for a text operand, and every `in` over a symbol or string
column fell through to the generic per-row hashset probe.

The kernel it could not reach carries a verdict-LUT written specifically
for SYM: one byte load per row regardless of set size.  It was dead code
for the only type that path was built for.

vec.h states the distinction where it defines the two predicates —
may_have_nulls is "a row-kernel gate, not a reason to reject a text
optimization", and "paths requiring null-free data use has_nulls below".
This is an admission check, so it now asks ray_vec_has_nulls, which
inspects the payload for text and is not on a per-row path.

Measured, 351,393-row SYM column, 766 distinct values, -c 2:

  needles        before      after
        1      4,820 us     200 us     24x
        2      5,180 us     160 us     32x
       10      8,320 us     160 us     52x
      100      8,660 us     180 us     48x
      766      8,440 us     180 us     47x
   50,000      7,360 us     520 us     14x

The reported symptom in #593 was the degenerate case — a one-element right
side costing ~120x the equivalent `(== col x)`.  That gap is now ~5x, but
the fix is not a small-needle special case: it lands on every text `in`,
which is why the whole column of numbers moves.

Null semantics are unchanged, and that is the risk the old gate existed to
manage: the kernel treats a null as matching nothing, while this path is
null-equals-null.  Admitting only exactly-null-free operands keeps the two
from being conflated — a null on EITHER side still falls through.  All
eight null shapes were compared against the pre-fix binary and are
identical; they are now pinned in test/rfl/collection/in.rfl, which had no
null coverage at all, together with the null-free shapes that now reach
the kernel and the degenerate single-needle case.

ClickBench (10M rows, 43 queries x 3, splayed): sum of per-query hot times
5690.7 ms -> 5664.6 ms, no query beyond +4%.  The exact check costs one
pass over each operand and does not show.

This site is the only one that had the defect.  Every other admission gate
reading may_have_nulls either excludes text by an explicit type guard below
it (ray_sorted_range_rowsel takes numeric and temporal columns only;
str_scalar_int's switch accepts no text type), handles text explicitly in
the same condition (fills_vec_eager returns text unchanged by design), or
reads the predicate in the direction where an always-true answer is the
safe one.  Checked rather than assumed, after an earlier draft of this
message claimed the opposite.

Refs #593

This branch has not been deployed

No deployments
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.

1 participant