andygrove opened a new pull request, #5086: URL: https://github.com/apache/datafusion-comet/pull/5086
## Summary Encode the ~10 recurring patterns that senior reviewers catch on Comet perf/correctness PRs into the `audit-comet-expression` and `review-comet-pr` skills, so contributors and reviewers can catch these locally before human review. Findings were derived by sweeping mbutrovich's review comments across ~50 andygrove PRs merged in the past 4 weeks and clustering the substantive feedback by theme. ## What changed **`audit-comet-expression`** — new "Rust-side test-hygiene checklist" (11 items) after the implementation-gaps section: - All eval modes (`Legacy`, `Try`, `Ansi`) exercised independently - Fast path AND fallback each hit by at least one input - Boundary values (`10^p - 1` vs `10^p`, `i64::MAX ± 1`, 38→39 digits, leap-day) - Positive AND negative overflow (guards against sign-asymmetric error text like `"too large"`) - NaN, +Inf, -Inf per eval mode - All-null / all-valid / all-invalid batches (`iter().all()` on empty) - First-offender ordering (batches with two different-looking offenders) - Error assertion depth: match on exact `SparkError` variant and all fields, never bare `is_err()` - Multi-limb / wide-value coverage for `i128` code - Fuzz-suite reach check (confirm the Scala generator actually reaches the new branch, e.g. digit-count cap) - Load-bearing invariants documented with `debug_assert!` or a comment **`review-comet-pr`** — new section 8 "Recurring Review Checks" with 10 numbered items, each tied to a real recent finding: 1. Diff-branch test coverage 2. Fast path AND fallback each hit 3. Error-assertion depth + sign symmetry 4. Comment / code alignment 5. Sibling call sites (fold in or file a linked follow-up) 6. Upstream arrow-rs / std API first; watch for `&dyn Fn` vs `impl Fn` asymmetry 7. Capacity hints vs `AVERAGE_STRING_LENGTH`, watch redundant headroom 8. Benchmark integrity (unique names, seeded RNG, no hallucinated benches, default-case measurement) 9. Panic-safety of table-lookup helpers (`10_i128.pow(40)` panics/wraps) 10. `debug_assert!` or comment for load-bearing invariants Plus a short "style tells to strip from Claude-authored diffs" note (`**bold**`, ALL-CAPS emphasis in comments, FQNs instead of `use`). ## Test plan - [ ] Docs-only change to skills under `.claude/skills/`; no runtime impact - [ ] Skill files still parse (frontmatter unchanged) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
