Coverage push + 5 bug fixes — group/query/cmp/agg#208
Merged
Conversation
…-query + serde roundtrip + traverse weighted 5 new RFL files, +475 assertions, all happy-path: - rfl/group/count_distinct_paths.rfl (49 assertions) Covers `ray_count_distinct_per_group` (serial), `count_distinct_per_group_buf` (per-group-slice low-cardinality), `count_distinct_per_group_parallel` (partition-by-gid 3-pass kernel — cdpg_hist_fn / cdpg_scat_fn / cdpg_dedup_fn). Single- and multi-key by; I64/F64/SYM/I32/I16/U8 val types. - rfl/ops/expr_typed_fast.rfl (114 assertions) Covers `binary_range` BR_AR_FAST arms (l_esz=8/4/2) for ADD/SUB/MUL/MIN2/MAX2; BR_FAST bool-cmp arms for I64/I32/I16/BOOL/SYM-W8; `par_binary_fn` parallel path at N>=65536; `par_binary_str_fn` STR EQ/NE/LT/LE/GT/GE; selection-aware par_binary_fn via nested select-where; DIV/IDIV/MOD generic arms. - rfl/ops/idiom_in_query.rfl (63 assertions) Covers `ray_idiom_pass` rewrites inside real query contexts (not bare exprs): count(distinct) in per-group agg slot, multi-key by, multiple idioms in one select, DAG-VM composed, OP_SCAN input vs computed input, null-bearing precondition slow-path, redirect_consumers correctness after rewrite, idiom identity preserved through predicate/projection pushdown passes. - rfl/store/serde_roundtrip.rfl (167 assertions) Covers ser/de for every atom type (BOOL/U8/I16/I32/I64/F64/SYM/STR/DATE/TIME/ TIMESTAMP/GUID — F32 not RFL-reachable), typed-null atoms, vectors of each type, sentinel-encoded null vectors (I64/F64/I32/I16/DATE/TIMESTAMP), slice vecs via (take ...), compounds (LIST/DICT/TABLE), and lazy materialise at ser boundary (asc/desc/reverse/distinct/sum/avg/min — must materialise before persisting per fix `f1c143b0`). - rfl/datalog/traverse_weighted.rfl (82 assertions) Covers `exec_dijkstra` (single-source + point-to-point early-exit + 4-arg max_depth), `exec_mst` + `mst_edge_cmp` (Kruskal on K4 / disconnected forest / DAG), `exec_random_walk` (deterministic dead-end + branching invariants), `exec_var_expand` ([min..max] depth ranges), `exec_shortest_path` (BFS hop-count), `exec_k_shortest` (Yen's K=2 / K=1), `exec_connected_comp` (1- and 2-component), `exec_expand` (1-hop). Tests: `make clean && make test` -> 2520 of 2522 passed (2 skipped, 0 failed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
reduction_i64_result in src/ops/group.c (the post-reduction wrapper that boxes the accumulator's int64 back into a typed atom) had cases for DATE/TIME/TIMESTAMP/I32/I16/U8 but missed RAY_SYM — so a SYM-typed column's min/max fell through to ray_i64(val) and the caller saw an i64 atom containing the sym's intern id instead of a SYM atom. Concretely: (min ['c 'a 'b 'a 'd]) -> 305 (type i64, was 'a as sym 305) (type (min ['x])) -> 'i64 (should be 'sym) (== (min ['z]) 'z) -> false (intern id != sym atom) Add the missing case: case RAY_SYM: return ray_sym(val); The TDD test test_rfl_agg_min_max_sym fails without this with `got "305", expected "x"` and passes after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`(select {m: (first (asc v)) by: k from: T})` and similar shapes
returned `error: domain` even though the equivalent `(min v)` works.
The DAG-level idiom pass in src/ops/idiom.c walks `inputs[]` only,
which never reaches the aggregator subtrees that live in
OP_GROUP's ext->agg_ins[]. As a result, (first (asc v)) /
(last (asc v)) / (count (asc|desc|reverse v)) survived into the
per-group dispatcher, which can't run a sort-wrapper inside an
aggregator slot — domain error.
Mirror match_count_distinct's pattern: add simplify_agg_idiom that
runs at the AST stage in the select-by planner, before agg_ins[]
is built. Same rewrites as src/ops/idiom.c's ray_idioms table:
(first (asc col)) -> (min col) if col is null-free
(last (asc col)) -> (max col) if col is null-free
(count (asc col)) -> (count col)
(count (desc col)) -> (count col)
(count (reverse col)) -> (count col)
The null-free precondition for first/last matches idiom.c's
pre_no_nulls_on_asc_input — `first(asc null-bearing)` returns null
(xasc puts nulls first) while `min(...)` skips nulls.
Add test/rfl/ops/idiom_in_select_by.rfl with assertions that
fail without this fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-group projection eval (nonagg_eval_per_group_core) stored each group's `cell = ray_eval(expr)` directly in the result LIST. When the inner expression returns a RAY_LAZY (e.g. (reverse v) wraps a fresh lazy chain), the cell is a deferred DAG node. Symptom: full-table display works (table-level fmt_obj walks each cell, calling ray_lazy_materialize), but extracting the column via (at table 'col) returns a LIST whose cells all read as `error: nyi`. Root cause: ray_lazy_materialize takes ownership of the graph — even if the lazy ray_t survives via shared refs, the graph itself is freed. After the first fmt of the whole table consumed the graph, every subsequent re-read of any cell hits a half-dead lazy whose execute fails inside the DAG VM with "nyi". Fix: materialise lazy cells eagerly in nonagg_eval_per_group_core before storing. Each cell is now a concrete typed-vec / atom — safe to read any number of times. Repro / regression test: test/rfl/query/list_col_at_extraction.rfl verifies both (a) full-table display and (b) repeated column-cell reads via (at (at table 'col) i) return the same concrete vec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ts + LIKE shapes + WHERE-AND chains 5 new RFL files, +314 assertions, all happy-path. Three findings documented inline (no test-routing-around). - rfl/group/reprobe_stress.rfl (30 assertions) Tests the per-group dispatch reprobe path that activates only when n_groups > 65536 (commit 91531da fix(group): per-group dispatch survives n_groups > 65536). Covers ray_median_per_group_buf, ray_topk_per_group_buf at >= 65k groups, reprobe_rows_fn, group_ht_insert_empty_group, group_rows_range_existing, group_probe_existing_entry. Multi-key (k1+k2) wide-key SYM arm also exercised. Single-thread baseline (~50k groups) confirms the smaller path still works. - rfl/datalog/graph_algos_advanced.rfl (48 assertions) PageRank (hub-dominance, rank-sum normalisation, 1/2/3-arg variants), Louvain (community detection), var-expand realloc paths (frontier 256->2048, out_count 1024->2048). Finding (documented inline, NOT routed around): Louvain returns 4 communities of size 2 for the canonical K4+K4+bridge fixture, not 2 — src/ops/traverse.c:1166 self-documents this as "Pass 1 only (no graph contraction)". Assertions encode the observed 4-communities output + the cross-cluster invariant that still holds; any future phase-2 addition will trip these and force a re-evaluation. Finding (out of scope): exec_astar is implemented at src/ops/traverse.c:2213 but has no RFL binding (no ray_graph_astar_fn, no .graph.astar registration). SCC has zero implementation in the tree. Both unreachable from RFL — needs source wiring before a regression test makes sense. - rfl/temporal/cross_cast_period.rfl (106 assertions) Cast matrix DATE/TIME/TIMESTAMP -> each other (atom + vector), boundary dates (epoch, Y2K, leap day, pre-Y2K, century-non-400 2100), DOW grid Mon-Sun, DOY for leap + non-leap. Covers ray_temporal_truncate atom + vec branches (int32 + int64 input), pre-epoch us<0 floor-toward-negative-inf arithmetic, rte_us_to_ts_raw, ray_*_clock_fn temporal-argument overload, cross-temporal casts in builtins.c (DATE<->TIMESTAMP, TIMESTAMP->TIME, String->DATE/TIME). Note: HAS_NULLS arm not reachable from literal vectors (needs csv load); date_trunc MINUTE/HOUR/MONTH/YEAR arms are dead at surface per ray_temporal_trunc_from_sym registering only DAY+SECOND. - rfl/strop/like_patterns.rfl (71 assertions) exec_like / exec_ilike compiled-shape branches: EXACT, PREFIX, SUFFIX, CONTAINS, ANY, GLOB (? char-class [...] multi-* mixed) x STR-vec + SYM-vec x like + ilike + scalar. Edge cases: empty pattern, empty input rows, pattern longer than every input, case-insensitive ilike on both pattern cases. Drives the SYM dict-cache branch (seen[]/lut[] first-touch + reuse). - rfl/query/where_and_chain.rfl (59 assertions, 1 XFAIL) Chained-filter compiler (commits 5205265 cost-based reorder, b406422 compile chained, 7f1d46e fused rgid_probe selection). 3- and 4-conjunct AND over 50k rows, cost reorder, reorder_safe=0 short-circuit-preserving guard, mixed agg+non-agg projection over a filtered rowsel, predicate pushdown past projection, semijoin via (in col ...), single-conjunct AND. Finding (XFAIL, Bug 4): `(and X)` single-conjunct returns error: domain. src/ops/query.c:4060 chained-filter branch requires ray_len(where_expr) >= 3 (and-head + >= 2 conjuncts), so degenerate (and X) falls through to compile_expr_dag which returns NULL. Planner should fold (and X) -> X before compile. Marked with !- domain and a paired (> v 100) form proving the un-wrapped predicate works. Tests: `make clean && make test` -> 2528 of 2530 passed (2 skipped, 0 failed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Programmatic AST construction like (cons 'and preds) previously broke when preds happened to have length 1: bare (and X) returned error: arity, and `where: (and X)` returned error: domain (the WHERE compiler at src/ops/query.c:compile_expr_dag had no n==2 branch for variadic AND/OR, so the LIST fell through to NULL → "WHERE predicate not supported by DAG compiler"). Switch to the monoid-identity rule from Scheme / Haskell: (and) -> arity error (no vacuous-truth element exposed) (and X) -> X (identity) (and X Y …) -> existing fold (or) -> arity error (or X) -> X (identity) (or X Y …) -> existing fold Two-site fix: - src/ops/cmp.c ray_and_vary_fn / ray_or_vary_fn: gate is now n>=1. For n==1 return the evaluated arg directly (skips the binary-fold setup). Both functions are RAY_FN_SPECIAL_FORM, so the n==1 case preserves short-circuit semantics by definition (nothing else to evaluate). - src/ops/query.c compile_expr_dag: handle n==2 case for (and X) / (or X) by returning compile_expr_dag(g, elems[1]). Sits above the existing n>=4 variadic tree-builder; the n==3 binary case is unchanged. Test churn (deliberately exposes the new contract): - test/rfl/cmp/and.rfl: (and true) was `!- arity`, now `-- true`. - test/rfl/cmp/or.rfl: (or false) was `!- arity`, now `-- false`. - test/rfl/query/where_and_chain.rfl: Bug 4 XFAIL block (`!- domain`) now passes as `-- 49899` / `-- 1249969950`. - test/rfl/cmp/and_or_identity.rfl (new): happy-path identity matrix for atom bool, vec bool, non-bool atom, nested (and (and X)), programmatic WHERE-clause construction. (and) and (or) with zero args remain arity errors — there is no language-level Boolean-monoid identity exposed for them, and the existing test pinning that case in and.rfl/or.rfl stays green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ip/holistic-aggs
5 new RFL files, +572 assertions, all happy-path. All paths exercised
through the public RFL surface; no de-static, no _probes/.
- rfl/temporal/parse_format.rfl (98 assertions)
ray_temporal_truncate atom + vec branches (DAY + SECOND, with the
typed-null arm: 0Nd/0Nt/0Np), ray_temporal_trunc_from_sym both
"date" / "time" symbol matches via dotted `.date`/`.time` in
select, exec_date_trunc DAG path (int32 source for DATE/TIME,
int64 source for TIMESTAMP), ray_date_clock_fn / ray_time_clock_fn /
ray_timestamp_clock_fn 'local + 'global symbol branches, idempotence
+ composition checks.
- rfl/ops/expr_mixed_types.rfl (147 assertions)
Type-pair combinations not in expr_typed_fast.rfl: INT-vec × F64-scalar
(and reverse), mixed-width int promotion (I16+I32, I16+I64, I32+I64,
I64+F64), vec-vec arith + comparisons across integer-family and
float-family arms, BOOL AND/OR vec-vec + vec-scalar, temporal arith
(DATE+I32, TIME+I32, TIMESTAMP+I64), DATE/TIME/TIMESTAMP comparisons,
SYM-vec × SYM-vec compare, STR-vec × STR-vec compare, IN at multiple
widths inside select. 5 documented dispatch quirks pinned with `--`
(each has a working alternative route exercised in the same file):
TIMESTAMP-vec needs explicit cast for scalar compare; (+ DATE DATE)
errors type (only date+offset supported); SYM-vec × STR works in
select context not at REPL; (in int-vec float-vec) returns all-false
standalone but works in select via use_double; (+ BOOL_vec U8_vec)
widens to I64.
- rfl/strop/strlen_partitioned.rfl (43 assertions)
Drives strlen_mapcommon (0% before) via .db.parted.get over partition
dirs naming a sym domain; strlen_parted (0% before) via parted SYM
data columns with multi-segment + mixed segment sizes. Both
dispatch arms in ray_strlen_fn (RAY_MAPCOMMON + RAY_IS_PARTED) now
hit by happy-path data.
- rfl/strop/string_manipulation.rfl (201 assertions)
exec_concat 2-6 arg SYM/STR/mixed; exec_substr scalar + per-row I64/I32
+ 1-element vec; exec_replace SYM + STR with exact-match / shrinking /
expanding / no-match / whole-string / pooled (>12-byte) cases;
exec_string_unary upper/lower/trim (lead/trail/both/interior/all-ws/
empty/no-pad/tab/newline); exec_strlen STR + SYM at lengths
{0,1,2,4,7,12,13,20,40,44}; pipeline (upper → substr → concat).
- rfl/agg/per_group_holistic.rfl (83 assertions)
med/median per-group at I64/I32/I16/U8/F64 sources; single-key I64,
multi-key SYM, multi-key I64; even/odd group sizes; ray_median_per_group_buf
parallel-path threshold (n_groups>=8, total>=4096) via 8192-row x
16-groups fixture. top/bot K per-group through ray_topk_per_group_buf
(SYM-keyed fall-through past rowform). var / var_pop / stddev /
stddev_pop / dev: canonical Wikipedia fixture per group; constant
group (var=0); 1-element group (sample = null, pop = 0); parallel
path; result-type always F64. Multi-agg combos: med+var_pop+count,
med+stddev, med+stddev 2-key SYM (hits the ms-fast-path at
query.c:6032), med+stddev+count (ms_with_count branch). Bug 5
pinned (per-group `dev` resolves to OP_STDDEV/sample while scalar
`dev` is OP_STDDEV_POP/pop — fix in follow-up).
Tests: `make clean && make test` -> 2534 of 2536 passed (2 skipped,
0 failed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug 5: scalar `(dev V)` and per-group `select {d: (dev v) by k}`
disagreed on the math.
- Scalar `dev` is registered in src/lang/eval.c:2527 as
`ray_dev_fn`, which calls `var_stddev_core(x, sample=0, sqrt=1)`
— POPULATION stddev (divides by n). Identical to `dev_pop` /
`stddev_pop`.
- Per-group `dev` was resolved in src/ops/query.c:316 to
OP_STDDEV — SAMPLE stddev (divides by n-1).
The same expression therefore returned different numbers in
different contexts:
(dev [2 4 4 4 5 5 7 9]) -> 2.0 (pop)
select{d:(dev v) by:k} on same group -> 2.138 (sample)
The mismatch isn't a math decision; it's two different authors
each picking the convention that felt natural to them. Scalar
side was Q-style (dev=pop), planner side was R/Excel-style
(sd=sample).
Fix: align planner with scalar by mapping `dev` -> OP_STDDEV_POP.
Now `dev` is a true alias of `stddev_pop` / `dev_pop` in every
context. Explicit `stddev` (-> OP_STDDEV) is still sample, so
users who actually need sample stddev have a clear name.
Test: test/rfl/agg/per_group_holistic.rfl had three assertions
pinning the old asymmetry (sum = 2*sqrt(32/7) and `dev != stddev_pop`
-> true). Flipped to pin the fixed contract (sum = 4.0, dev ==
stddev_pop -> true). No other test references `dev` in a
per-group select.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ser-vasilich
changed the title
5 tasks
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
2 participants
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
Coverage push across multiple rounds, plus 5 real source bugs surfaced and fixed along the way using TDD (failing test → fix → passing test).
Tests: 2519 baseline → 2534 of 2536 PASS, 0 failed (2 skipped: 1 pre-existing, 1 from PR #207's
public/vec_get_sym_id_w8happy-path narrowing).Coverage (vs PR #207 merged baseline):
Per-file high-impact deltas:
src/ops/strop.csrc/ops/group.csrc/ops/cmp.csrc/ops/string.csrc/ops/query.csrc/ops/temporal.cBug fixes (TDD: failing test → fix → passing test)
Bug 1:
(min/max SYM_vec)returned i64 sym-id, not SYM atomsrc/ops/group.c:reduction_i64_resulthad cases for DATE/TIME/TIMESTAMP/I32/I16/U8 but missedRAY_SYM→ fell through toray_i64(val). Addedcase RAY_SYM: return ray_sym(val);. Regression intest/rfl/agg/min_max_sym.rfl.Bug 2:
(first (asc v))etc. in select-by aggregator →error: domainThe DAG-level idiom pass in
src/ops/idiom.cwalksinputs[]only — it never reaches aggregator subtrees that live inOP_GROUP'sext->agg_ins[]. Added an AST-level rewrite (simplify_agg_idiominquery.c) mirroring the DAG idioms before agg_ins is built. Regression intest/rfl/ops/idiom_in_select_by.rfl.Bug 3:
(at table 'list-col)after per-group projection returned[error: nyi × N]nonagg_eval_per_group_corestored each cell as aRAY_LAZYray_t. The firstfmt_objof the table consumed the lazy's graph; every subsequent cell read hit a half-dead lazy whose execute failed with "nyi". Fix: eagerly materialise lazy cells before storing in the result LIST. Regression intest/rfl/query/list_col_at_extraction.rfl.Bug 4: single-arg
(and X)/(or X)returned arity / domainProgrammatic AST construction like
(cons 'and preds)broke whenpredshad length 1. Switched to the monoid-identity rule (Scheme/Haskell):(and X) == X,(or X) == X. Zero args still rejected. Fixed at both eval level (cmp.c) and DAG-compile level (query.c compile_expr_dag). Regression intest/rfl/cmp/and_or_identity.rflplus updated docs incmp/and.rflandcmp/or.rfl.Bug 5:
devhad different math in scalar vs select-by contextsScalar
(dev V)was POPULATION stddev. Per-groupselect{d:(dev v) by k}mapped toOP_STDDEV→ SAMPLE stddev. Aligned planner with scalar (query.c:316:dev → OP_STDDEV_POP).devis now a true alias ofstddev_pop/dev_popeverywhere. Regression intest/rfl/agg/per_group_holistic.rfl.New tests (~15 RFL files, ~1800 assertions)
Aggregators:
min_max_sym,rowform_topk/maxmin/sum_count,variance,per_group_holistic,count_distinct_paths,reprobe_stress.Query / planner:
idiom_in_select_by,idiom_in_query,list_col_at_extraction,per_group_buf,parallel_probe,where_and_chain,expr_typed_fast,expr_mixed_types,and_or_identity.Strings / temporal:
strlen_partitioned,string_manipulation,like_patterns,string_par,extract,arith,cross_cast_period,parse_format.Storage / graphs / serialization:
serde_roundtrip,fused_topn,traverse_weighted,graph_algos_advanced,hof/wrappers,csv_splayed.Test plan
make clean && make test(debug, ASan+UBSan): 2534 of 2536, 0 failed_probes/, no hidden xfail — observed bugs either fixed or pinned with--assertions in the visible pipelinesrc/test-only de-staticing, no internal headers added for testsObservations documented (not bugs in this PR's scope)
.graph.louvainreturns 4 communities on a K4+K4+bridge fixture (instead of canonical 2) because the contraction step is unimplemented. Pinned ingraph_algos_advanced.rfl. Sourcesrc/ops/traverse.c:1166self-documents this.exec_astaris implemented but has no RFL binding. SCC has zero implementation. Flagged as unreachable from RFL.--assertions; each has a working alternative route exercised in the same file.Out of scope (next coverage round)
🤖 Generated with Claude Code