feat: column roles gate all value-distribution detectors (1.1.0)#63
Conversation
Role-based skipping now applies to every value-distribution detector, not just point.modz: ctx.seasonal, coll.cusum, dist.ks/psi/chi2, and mv.mahalanobis skip identifier/sequence columns (mv excludes them from the feature matrix). A seasonal subseries / level-shift / drift test / joint distance over arbitrary ids or a monotonic ramp is noise — this closes the gap noted in 1.0.1 where coll.cusum still flagged a syslog procid shift. struct.schema stays role-agnostic (null-rate/schema-diff meaningful for any column); cad.regularity uses only the explicit --cadence column. Shared Role::skips_value_detection() centralizes it. Changes detector output for column_roles=true → config_version bumped anomalyx-cfg/8→/9. Envelope/PROTOCOL unchanged; --no-column-roles restores pre-roles behavior everywhere. Goldens re-blessed for the new fingerprint. Gates: proptest + cargo-mutants 0-missed on all 6 changed files (roles 37 caught; ctx/coll/mv 109; point/dist 187 + 10 loop-bound timeouts). Added role-skip tests per detector. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 48 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces role-aware column filtering across value-distribution detectors. A new ChangesColumn Role-Aware Detector Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ax-detect/src/ctx.rs (1)
102-107: 💤 Low valueSuggest checking numeric type before role for efficiency.
The role check precedes the numeric type check, which means
col.role()is computed for non-numeric columns even though they're ignored by the subsequentis_numeric()check. Sincerole()is computed on each call (not cached), this is a minor inefficiency.For consistency with other detectors (e.g.,
dist.rs:209,coll.rs:64,mv.rs:46) and to avoid unnecessary computation, consider reordering:fn scan_all(det: &SeasonalDetector, rs: &RecordSet, cfg: &DetectConfig, out: &mut Report) -> bool { let mut assessed = false; for col in &rs.columns { + if !col.ty.is_numeric() { + continue; + } // Skip identifier/sequence columns: a seasonal subseries of arbitrary // labels or a monotonic ramp is meaningless. Roles still ship in the // envelope; `column_roles = false` disables this. if cfg.column_roles && col.role().skips_value_detection() { continue; } - if col.ty.is_numeric() { - assessed |= det.scan_column(col, cfg, out); - } + assessed |= det.scan_column(col, cfg, out); } assessed }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ax-detect/src/ctx.rs` around lines 102 - 107, Reorder the checks so you skip non-numeric columns before calling the potentially-expensive role() method: first check col.is_numeric() and continue if false, then (if cfg.column_roles) call col.role() and use role().skips_value_detection() to decide to continue; i.e., change the current conditional order around cfg.column_roles, col.role(), and col.is_numeric() to call col.is_numeric() before col.role().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ax-detect/src/ctx.rs`:
- Around line 102-107: Reorder the checks so you skip non-numeric columns before
calling the potentially-expensive role() method: first check col.is_numeric()
and continue if false, then (if cfg.column_roles) call col.role() and use
role().skips_value_detection() to decide to continue; i.e., change the current
conditional order around cfg.column_roles, col.role(), and col.is_numeric() to
call col.is_numeric() before col.role().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9206c978-f406-49ce-a3eb-8d23f6a0ce41
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdCargo.tomlcrates/anomalyx/tests/golden/describe.jsoncrates/anomalyx/tests/golden/scan_basic.jsoncrates/ax-core/src/roles.rscrates/ax-detect/src/coll.rscrates/ax-detect/src/config.rscrates/ax-detect/src/ctx.rscrates/ax-detect/src/dist.rscrates/ax-detect/src/mv.rscrates/ax-detect/src/point.rs
The magic-prefixed-garbage test fed crafted length fields to binary container decoders (parquet/arrow/avro/orc/evtx/pcap) that delegate to crates trusting those lengths — a fuzzed length could make the dependency attempt a ~21GB allocation (intermittent CI OOM). That is a binary-format-parsing property, not an anomalyx logic bug, and isn't boundable without forking the decoders. Scoped MAGICS to sqlite (allocation bounded by the input buffer) and documented the limitation; those parsers are still fuzzed via arbitrary bytes (rejected at the magic check). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
What
Extends column-role awareness from
point.modzalone to every value-distribution detector.ctx.seasonal,coll.cusum,dist.ks/dist.psi/dist.chi2, andmv.mahalanobisnow skipidentifierandsequencecolumns (mv excludes them from its feature matrix). A seasonal subseries, level-shift, drift test, or joint distance over arbitrary ids or a monotonic ramp is noise, not signal.This closes the gap noted in 1.0.1: on real syslog,
coll.cusumstill flagged a shift in theprocidcolumn. Now it doesn't.Out of scope (deliberately):
struct.schemastays role-agnostic — null-rate and schema-diff are meaningful for any column — andcad.regularityonly ever operates on the explicitly-named--cadencecolumn.A shared
Role::skips_value_detection()(Identifier | Sequence) centralizes the rule;pointwas refactored onto it too.Contract
Changes detector output when
column_roles = true, so theconfig_versionfingerprint is bumpedanomalyx-cfg/8 → /9(per the 0.8.0 precedent). Envelope shape andPROTOCOLunchanged;--no-column-rolesrestores pre-roles behavior across all detectors. Goldens re-blessed for the new fingerprint (the only diffs:config_version, andmv's absence reason now that theidcolumn isn't a feature — caught by the golden tripwire).Gate
proptest+cargo-mutants0 missed across all 6 changed files (roles.rs37 caught;ctx/coll/mv109;point/dist187 + 10 loop-boundks_statistictimeouts). Added a role-skip test per detector (each: roles-on skips,--no-column-rolesassesses).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores