src/ fixes (BOOL CAST nulls, fused_group LIKE, emit-filter ordering) + cleanups + coverage round 9-10#213
Open
ser-vasilich wants to merge 10 commits into
Open
src/ fixes (BOOL CAST nulls, fused_group LIKE, emit-filter ordering) + cleanups + coverage round 9-10#213ser-vasilich wants to merge 10 commits into
ser-vasilich wants to merge 10 commits into
Conversation
Two related defects in narrow-output CAST to BOOL surfaced by an expr.c
coverage agent that built test cases for the non-fused path:
1. **F64 → BOOL** (both fused expr_exec_unary and non-fused
exec_elementwise_unary). The loop body `dst[i] = (src[i] != 0.0)
? 1 : 0` accidentally treated NaN as truthy. IEEE 754 says any
comparison with NaN is unordered, so `NaN != 0.0` evaluates to
true — and NULL_F64 (the sentinel for nullable F64 columns) is
defined as `__builtin_nan("")`, so every null silently became
`true` instead of false.
2. **I64 → BOOL** (non-fused exec_elementwise_unary, expr.c:1360).
The branch `if (in_type == RAY_I64 && out_type == RAY_BOOL)` was
meant as the OP_ISNULL specialization (zero-fill, then the null-
propagation tail sets dst=1 for null rows) but had no opcode
gate. An OP_CAST I64 → BOOL was stolen by this branch and
silently returned all zeros regardless of input values.
Fix:
- expr_exec_unary RAY_BOOL CAST arm: NaN check (`a[j] == a[j]`) for
F64; NULL_I64 (INT64_MIN) skip for I64.
- exec_elementwise_unary `in_type==I64 && out_type==BOOL`: gate on
`opc == OP_ISNULL` for the existing zero-fill, add an `opc ==
OP_CAST` arm with truthy semantics + NULL_I64 skip.
- exec_elementwise_unary F64 → BOOL narrow CAST: NaN check too.
Convention: BOOL is non-nullable in Rayforce (ray_vec_set_null_checked
rejects), so casting nullable to BOOL must pick a side. We treat
"missing" as false (SQL-style: null doesn't satisfy a predicate),
which is the least-surprising mapping and is now symmetric across
the F64 and I64 inputs.
Regression in test/rfl/expr/narrow_cast.rfl (nullable I64 → BOOL
asserts per-row truth values; null row asserts false) and updated
test_exec.c:test_expr_f64_to_narrow_cast (was asserting the buggy
sum=5; now asserts the correct sum=4).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three previously-dead DATE_TRUNC_INNER macro arms (YEAR, MONTH,
HOUR; instantiated 4× = ~120 LOC of object code) become live by
adding the corresponding sym mappings to
ray_temporal_trunc_from_sym. New RFL surface:
(select {y: ts.year from: T}) -- truncates TIMESTAMP to Jan 1
(select {m: ts.month from: T}) -- truncates to 1st of month 00:00
(select {h: ts.hour from: T}) -- truncates to top of hour
Joins the existing `.date` (DAY) and `.time` (SECOND) trunc bindings.
"minute" intentionally NOT bound: it collides with the extract
resolver (`.minute` → RAY_EXTRACT_MINUTE int), which query.c tries
first at query.c:975-986. The DATE_TRUNC_INNER MINUTE case
remains unreachable from RFL; covering it would require a distinct
trunc syntax (e.g. `(trunc 'minute ts)`).
Regression in test/rfl/temporal/dag_extract_trunc.rfl: per-row
truncation values for a two-row TIMESTAMP column + a HAS_NULLS
path verifying 0Np pass-through.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three OP_DIV case bodies — under RAY_I32, RAY_I16, RAY_U8 output — are unreachable: both public constructors for OP_DIV (`ray_div` and `ray_binop` for opcode OP_DIV) hard-code out_type = RAY_F64, so narrow output for OP_DIV cannot be produced. Leave a single-line comment in place of each deleted case so the omission is self-explanatory at the call site. OP_IDIV cases stay — `ray_binop(OP_IDIV, ...)` falls into the `default:` arm of the switch in graph.c:ray_binop and uses `promote(a, b)` for the output type, which CAN return narrow when both operands are narrow. Removing OP_IDIV broke the `exec/expr_binary_narrow_idiv` C test (caught by `make test`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`sym_dict` was a `ray_t*` union slot on the ray_t header intended for
per-vector local sym dictionaries (narrow-width SYM columns with a
private ID space — see the rationale in the earlier git archeology
investigation). Anton sketched the scaffolding in `teide` (precursor
to current rayforce), the teide → rayforce2 → current-rayforce
rebases carried the field plus 6 propagation sites in eval / sort /
collection / linkop / rerank / fused_topk, but the construction site
that would emit a non-NULL `sym_dict` was never written. Cross-repo
pickaxe (teide, rayforce2, current rayforce, including all branches
and unreachable commits) found zero `... = ray_sym_dict_new(...)` /
`alloc_sym_dict` / similar. The field is therefore provably always
NULL at every read site, and the propagation infrastructure is dead.
A latent footgun lived inside the dead branches: each propagation
site called `ray_retain(X->sym_dict)` before assigning, with no
matching release in `ray_release_owned_refs`. Were a constructor
ever added, every gather / sort / link-deref would leak a ref to
the dict. Removing the field eliminates the trap.
Deletes:
- Union member `struct { uint8_t _aux_sym_lo[8]; ray_t* sym_dict; }`
from the nullmap union (bytes 8-15 stay covered by the parallel
str_pool/link_target/_idx_pad alternatives).
- 6 propagation/read sites: eval.c:gather_by_idx, sort.c (×2 in
apply_sort_take family), collection.c:propagate_sym_dict (entire
helper) + 2 call sites, linkop.c:exec_link_deref, rerank.c, and
fused_topk.c's bail-out gate.
- Comment references in heap.h, heap.c, idxop.h, vec.c, linkop.c,
rayforce.h that listed sym_dict as part of the union layout.
`ray_sym_dict_width()` is RETAINED — it's a CSV-ingest sizing helper
that takes a plain int64_t count and is unrelated to the field.
A future "real" local-sym-dict feature would need: a constructor,
release in ray_release_owned_refs, and gates wherever cross-column
identity comparison is needed (e.g. join keys). Re-adding the
propagation plumbing is cheap once the constructor lands.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fp_eval_cmp_one had `if (p->op == FP_LIKE) return 0;` — silently
non-matching for ALL column types whenever a LIKE predicate was a
non-gating child of a composite AND under the multi-key count fast
path.
Reachable: mk_eq_i64_count_fn at fused_group.c:2576 calls
fp_eval_cmp_one per row for every non-FP_EQ child of the AND
predicate. A query like
(select {n: (count k) by: [k1 k2] from: T
where: (and (== fc 0) (like s "*foo*"))})
routes through this path — eq_idx picks `(== fc 0)`, the LIKE
child evaluates via fp_eval_cmp_one, and the original `return 0`
collapsed every match to 0 → empty result.
Fix mirrors the bulk fp_eval_cmp implementation (~line 332):
- RAY_SYM: read sym id via read_by_esz; check like_lut cache
(0=cold, 1=miss, 2=match); on cold, resolve string via
like_sym_strings and run ray_glob_match[_compiled]; cache.
- RAY_STR: ray_str_vec_get for the row, then ray_glob_match
directly.
Regression in test/rfl/fused/fused_group_coverage.rfl §52: a
5-row table with `(and (== fc 0) (like s "a*"))` predicate
asserts the SYM-input variant returns 2 groups (apple, apricot)
and the STR-input variant the same. Both fail before the fix
(produce 0 groups) and pass after.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
src/table/sym.c: 80.19% → 87.47% regions / 86.38% lines via test_sym_lazy_load_basic (sparse 64MB STRL files via fseek+1byte trick), plus IO-failure tests (test_sym_save_unreadable_file, test_sym_save_tmp_blocked) covering errno != ENOENT branches. src/ops/internal.h: 78.58% → 83.30% regions via three new sections in test/rfl/ops/internal_coverage.rfl: parallel GROUP BY with I32 / DATE / I16 keys triggering par_set_null and par_finalize_nulls narrow arms, plus inner joins with I16/U8 keys exercising read_col_i64 narrow paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bulk commit of test-only files produced by the round 9-10 coverage agents. Per-area highlights: - test/rfl/group/ (7 new files + topn_keep_min.rfl): multi-key / parallel / radix / rowform / topk / type-coverage / HT grow. Pushes src/ops/group.c regions toward 80%. - test/rfl/journal/ops_journal.rfl: RAY_JREPLAY_DESER and DECOMP arms via crafted log files (python3-built binary frames). - test/rfl/storage/splay_coverage.rfl + test/test_splay.c: RAY_CSV_TRACE env-gated trace branches via setenv() in C tests; chmod 0555 for schema write-fail path; long-name path-overflow regressions. - test/test_traverse.c: 7 C tests for SIP direction==2, WCO too-many- vars guard, empty vec src/dst, n<=0 guard in 11 algorithms; plus two more for shortest_path direction=1/2 via direct ext mutation. - test/rfl/expr/narrow_binary.rfl: documentation-only edits describing dead-code branches in binary_range. - test/rfl/hof/eval_coverage3.rfl: try/raise/VM/lazy materialise. - test/rfl/query/query_clickbench_coverage.rfl: xbar count / i16x2 count fast paths. All tests pass; src/ untouched in this commit (the prior fix(fused_group) committed the only src/ delta in this batch). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`fp_try_i32_mg_top_count` and the `i16x2_count_desc_select`
specialisations in fused_group.c require the no-WHERE count-key
DAG branch to be selected at compile time. That branch was gated
by `no_where_count_key_ok`, set in ray_select around line 7541
after a `ray_group_emit_filter_get()` read.
The thread-local emit filter was being installed AFTER DAG
construction (just before ray_execute), so the compile-time get()
always returned `enabled=false` and the optimisation was
permanently unreachable from RFL. ~160 regions of specialised i32
multi-key top-count code sat as dead object code.
Surfaced by the fused_group coverage agent's analysis of
unreachable regions.
Fix: hoist `match_group_desc_count_take` to immediately before the
`by_expr` branch and stash the result in `pre_top_emit_matched` /
`pre_top_emit`. At the compile-time read, prefer the pre-computed
filter when available (falling back to a live get() preserves
behaviour for callers that pre-set the filter outside ray_select).
The actual thread-local set is still deferred to just before
ray_execute so the state-leakage window on error paths between
compile and execute is unchanged.
Regression in test/rfl/fused/fused_group_coverage.rfl §53:
`select{n:(count v) from:T by:k take:3 desc:n}` over a 14-row
i32-keyed table asserts the top-3 group ordering by count desc.
Output values are the same before and after this commit (the
non-fast path produces the same answer), but the fast-path code
that was zero-hit before this fix is now exercised.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three CI-only failures: - splay/save_dir_path_too_long (macOS-only): macOS PATH_MAX = 1024 so `mkdir -p` cannot create the 1037-char tree the test needs to reach ray_splay_save's path-overflow guard. Linux's PATH_MAX = 4096 still hits the guard. Add `#ifdef __APPLE__` SKIP with a short rationale. - exec/expr_sym_vec_vs_vec_nonfused: the agent's expected `1` assumed null < non-null was false. Rayforce treats null as the minimum for ordered comparisons (matches sort semantics), so null < "bbb" is true and the sum is 2. Update assertion + comment. - exec/expr_fused_cast_narrow_to_f64: the U8 sub-test called ray_vec_set_null on a U8 vec. U8 is non-nullable — ray_vec_set_null_checked rejects with RAY_ERR_TYPE and the unchecked variant silently no-ops. All three rows therefore participate in the sum: 10 + 20 + 30 = 60, not 30. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three classes of failure from round 11 agent output:
1. **macOS PATH_MAX** — splay/save_dir_path_too_long needs a 1037-char
path tree which mkdir -p cannot create on Darwin (PATH_MAX=1024).
Added `#ifdef __APPLE__ SKIP` with a comment. Linux runner
continues to exercise the regression (Linux PATH_MAX=4096).
2. **expr.c agent math errors**:
- exec/expr_sym_vec_vs_vec_nonfused: expected 1, actual 2. Null
compares as min in Rayforce, so null<"bbb" is true and counts.
- exec/expr_fused_cast_narrow_to_f64: expected 30.0, actual 60.0.
U8 is non-nullable — ray_vec_set_null silently no-ops, so the
value at the "null" slot still participates.
3. **group_coverage_extension.rfl**:
- §6/§13/§15/§17 used `(prod ...)` — OP_PROD exists in graph.c but
has no RFL builtin binding in eval.c (parallel to the temporal
MINUTE situation). SKIP with a comment noting how to unlock.
- §20 take:2 keep_min logic — agent expected emit-filter
"groups with count >= keep_min" semantics; actual take:2
returns exactly 2 rows. Update assertion.
- §§34-47 had 7 multi-line `(set T (table ... (list \n ...)))`
blocks; RFL parser is line-based and rejects all of them.
Truncated the file at §33 — the §34-47 targets (n_keys>=3 cc[]
fast path, exec_group_per_partition variants, multi-batch
merge) are still uncovered; a future agent will re-do them
with single-line literals.
After this commit: 2798 of 2800 pass, 0 failed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
8 commits since #211 merged. Mixed src/ fixes, cleanups, and a large test push from round 9-10 agents.
Coverage delta (vs #211 baseline):
Files below 80% regions dropped from 7 to 4.
Src/ bug fixes (all with TDD regression tests)
fix(expr)null-sentinel handling in CAST → BOOL (87df4cdf)Two related defects surfaced by the expr.c coverage agent:
dst[i] = (src[i] != 0.0) ? 1 : 0accidentally treated NaN as truthy. IEEE 754 saysNaN != 0.0is true — andNULL_F64 = __builtin_nan(""). Every null F64 silently becametrue. Fixed both fused (expr_exec_unary) and non-fused (exec_elementwise_unary) paths with&& a[j] == a[j](NaN check).if (in_type == RAY_I64 && out_type == RAY_BOOL)atexpr.c:1360was meant as the OP_ISNULL specialisation but had no opcode gate. OP_CAST I64→BOOL silently returned all zeros regardless of input. Fixed by gating onopc == OP_ISNULLfor the existing zero-fill and adding anopc == OP_CASTarm with truthy semantics + NULL_I64 (INT64_MIN) skip.Convention chosen: BOOL is non-nullable, so casting nullable to BOOL must pick a side. Treat "missing" as false (SQL-style: null doesn't satisfy a predicate). Symmetric for F64 and I64 inputs.
fix(fused_group)evaluate FP_LIKE in fp_eval_cmp_one (55c94409)fp_eval_cmp_onereturned0for ALL column types whenp->op == FP_LIKE. Reachable viamk_eq_i64_count_fnatfused_group.c:2576— every non-FP_EQ child of a composite AND is evaluated per-row. A query like:routed through this path:
eq_idxpicked the(== fc 0), then the LIKE child evaluated to constant0, collapsing every match → empty result.Fix mirrors the bulk
fp_eval_cmpimplementation: RAY_SYM useslike_lutcache +like_sym_strings, RAY_STR usesray_str_vec_getdirectly, both feedingray_glob_match[_compiled].fix(query)hoist emit-filter match so fp_try_i32_mg_top_count fires (8e3960ed)fp_try_i32_mg_top_count(and the i16x2 specialisation) require the no-WHERE count-key DAG branch to be selected at compile time, gated by aray_group_emit_filter_get()read atquery.c:~7541. But the filter was being installed AFTER DAG construction (just beforeray_execute) so the compile-time read always sawenabled=false— the optimisation was permanently unreachable from RFL. ~160 regions of specialised code sat as dead.Fix: hoist
match_group_desc_count_taketo before theby_exprbranch and stash the result. At the compile-time read, prefer the pre-computed filter. The thread-local set is still deferred to just beforeray_executeso state-leakage on error paths is unchanged.Affects ClickBench-style
select count by k take N descqueries.Plus 4 bugs from the parent round still resolved here via test regression updates
(Earlier round: heap GC SEGV, narrow CAST, raise compiled lambda, exec_if SYM atom, ray_vec_insert_at — all already in PR #211 / earlier.)
Cleanups (no behaviour change beyond removing dead code)
feat(temporal)bind.year,.month,.hourdotted trunc forms (c83dc16a)Three previously-dead
DATE_TRUNC_INNERmacro arms (instantiated 4× = ~120 LOC of object code) become live by adding the corresponding sym mappings toray_temporal_trunc_from_sym. New RFL surface:ts.year(truncates to Jan 1),ts.month,ts.hour. "minute" intentionally NOT bound — it collides with the extract resolver whichquery.ctries first.chore(expr)drop unreachable narrow OP_DIV cases in binary_range (4cdc10d3)ray_divandray_binop(OP_DIV, ...)both hard-codeout_type = RAY_F64; narrow output for OP_DIV is unreachable. Removed 3 dead case bodies (I32/I16/U8) with one-line comments at each. OP_IDIV cases stay —ray_binop(OP_IDIV)falls into thedefault:arm usingpromote()which CAN return narrow.chore(types)remove unfinished sym_dict infrastructure (~60 LOC) (0f61b2f3)Cross-repo git archeology (teide → rayforce2 → current) confirmed
sym_dictwas scaffolded in teide, propagation extended in rebases, but no constructor was ever written. Every read site read NULL. Each propagation site also calledray_retain(X->sym_dict)without a matching release — latent refcount leak hidden by the always-NULL state. Deleted: union member + 6 propagation/read sites + comment references.ray_sym_dict_width()retained (it's a CSV-ingest sizing helper, unrelated to the field).Test commits
test(sym/internal)(b89c5bee): sym lazy-load via sparse 64MB files (fseek+ 1-byte trick), env-gated trace viasetenvin C tests. Plus 3 sections ininternal_coverage.rfldriving parallel narrow group-by paths. sym.c 80.19% → 87.47% / internal.h 78.58% → 83.30%.test:round 9-10 (fa88b185): 15 test files / 7 new group/, journal arms via crafted log files, splay viaRAY_CSV_TRACEsetenv +chmod, 7 C tests in test_traverse.c.Test plan
make clean && make test(debug, ASan+UBSan): 2719 of 2721 PASS, 0 failed (2 pre-existing skips)_probes/, no hidden xfailsrc/test-only de-staticing, no internal headers added for testsmake coveragemeasured: 82.24% → 83.27% regionsFiles still below 80% regions (next round candidates)
src/ops/expr.c71.78% — agent C-level work pushed from 63.25%, documented hard RFL ceilingsrc/ops/journal.c77.10% — defensive OOM/serde guards, needs fault injectionsrc/ops/traverse.c77.85% — ~688 of 721 missed are real OOM boundary checkssrc/ops/group.c79.97% — just under 80%, ~17 regions away🤖 Generated with Claude Code