Skip to content

Agent/indexed statistical groups#325

Merged
singaraiona merged 5 commits into
devfrom
agent/indexed-statistical-groups
Jul 10, 2026
Merged

Agent/indexed statistical groups#325
singaraiona merged 5 commits into
devfrom
agent/indexed-statistical-groups

Conversation

@singaraiona

Copy link
Copy Markdown
Collaborator

What & why

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

Restore allocator GC and add .mem.objsize and .mem.ts. Share query timing, memory, and worker metrics with profiling and query logging.
@github-actions

Copy link
Copy Markdown

Rayforce targeted audit failed

This PR did not pass the required Rayforce audit gate. Fix the blocking findings below and push an update; the audit will rerun automatically.

Review complete. Here is the audit report.


Rayforce PR 325 Audit — Fast Merge Gate

Scope: git diff origin/dev...HEAD (head ab4e864f, 3 commits: part-index filter routing, scoped query measurement / .mem.*, fused indexed statistical groups + trailing window frames). Reviewed the full C diff plus adjacent code (idxop.c sort/hash helpers, exec.c filter dispatch, generic group emit for corr/cov parity, RAY_ATTR_SORTED maintenance). No builds or PR code executed, per CI rules.

Blocking findings

1. Competitor product ("q"/kdb+) references introduced in source and docs

  • src/ops/system.c:463 (new comment block for .mem.objsize): "useful as the Rayforce analogue of q's .mem.objsize for materialised query results."
  • docs/docs/namespaces/mem.md (new file, peak-live-bytes row): "This is the closest analogue of q's .Q.ts space value."

Rayforce policy neutralizes competitor product names in code, comments, and docs (benchmark names are the only sanctioned exception). These are direct references to the kdb+/q product and its API, newly added by this PR. Both sentences can simply be deleted without losing technical content.

2. frame: 'running window MIN/MAX regressed from O(n) to O(n²) per partition

src/ops/window.c:325-370 — the RAY_WIN_MIN/RAY_WIN_MAX merge rewrote the non-whole branch as a per-row rescan:

int64_t lo = trailing ? i - frame_start_n : ps;
...
for (int64_t j = lo; j <= i; j++) { ... }

For the trailing frame this is bounded by the frame width and fine. But for the pre-existing running frame ('running, UNBOUNDED PRECEDING .. CURRENT ROW), lo == ps, so every row rescans the partition prefix — O(n²) per partition. The code this replaced maintained a monotone running best in a single pass (the running frame only ever grows, so incremental min/max is trivially correct). A running-min over a 1M-row partition goes from ~1M to ~5×10¹¹ element visits — a catastrophic, silent slowdown of an existing documented feature. The new window.rfl test exercises trailing frames only on a 6-row table, so the suite cannot catch this. Fix: keep the incremental accumulator for the running case and use the rescan (or a deque) only for trailing.

Non-blocking observations (no action required for merge, worth follow-up)

  • ray_rowsel_intersect defensive branch writes before rewinding (src/ops/rowsel.c, fill pass): when a MIX∩MIX segment yields pc == seg_len, the counting pass excludes it from idx_count, but the fill pass writes all seg_len ids into oi before detecting the case and rewinding — a buffer overrun if a non-canonical (fully-populated MIX) rowsel ever reaches it. Unreachable under the current canonical-encoding invariant (the comment says as much), but a "defensive" path that is itself unsafe should either validate up front or reserve the space.
  • NaN F64 key on the new ray_sorted_range_rowsel (src/ops/idxop.c): col > NaN binary-searches to [0, n) and selects all rows. This exactly mirrors the pre-existing ray_index_range_rowsel sort-index route, so it is index-route parity, not a new divergence — but both routes plausibly disagree with the scan kernel's NaN semantics; worth a targeted follow-up test.
  • .sys.gc changes from documented no-op to a real allocator maintenance pass (src/ops/system.c). Intentional, documented, and covered by objsize.rfl; flagging as a behavior change reviewers should be aware of.
  • ray_query_measure_begin resets qstats slots (src/core/qmeasure.c): a .mem.ts nested inside a query-logged/profiled evaluation resets the outer scope's worker-busy counters, under-reporting the outer row's parallelism. Telemetry-only.
  • The core correctness surface otherwise checks out: the new slice-group corr/cov/scov/wsum/wavg emit formulas and their edge cases (cnt<2 → null, sx==0 wavg → null, dx<=0 corr → null) are byte-for-byte the generic HT path's; part-index span rowsels validate span ordering/totals and fall back to scan on any inconsistency; the sorted-range fast path is properly gated (RAY_ATTR_SORTED now cleared on every payload mutation in vec.c) and falls through cleanly on intersect failure; new tests cross-check part-indexed, hash-indexed, sorted, and plain layouts against each other.

Verdict

Two merge-blocking issues: a hard-rule vendor-mention violation (trivial to fix) and an asymptotic performance regression in running-frame window min/max.

RAYFORCE_AUDIT_VERDICT: FAIL

@singaraiona singaraiona merged commit fc7a7b1 into dev Jul 10, 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.

1 participant