sample: review-driven cleanup, fix streaming bernoulli header bug, dead retry loop, cluster pre-alloc

[jqnatividad]

Summary

Review-driven cleanup of src/cmd/sample.rs (net -44 lines, 8 sampling methods touched). Two commits:

1. 98693bdff — initial review pass
2. d6ba2ea2a — addresses roborev build(deps): bump simple-expand-tilde from 0.1.4 to 0.1.5 #1767 follow-up review

Bugs fixed

• Streaming Bernoulli over URL corrupted output in two ways:
  • The header was split on the first raw \n, so an RFC 4180 header with a quoted newline truncated the headers and shifted every record. Fixed by csv-driven boundary detection (Reader::position().byte()); same fix applied to the data loop's rposition('\n'). Trailing partial records are held back until either more data arrives or the stream ends naturally.
  • --no-headers was passed straight into has_headers(...) (inverted), discarding the first data row. Fixed.
  • --bernoulli 1.5 https://… reached random_bool() and panicked because validation lived after the URL dispatch. Validation hoisted to the top of run() (also rejects NaN/Inf sample sizes).
  • --max-size was silently ignored on the streaming path. Now honored; capped buffers do not flush a final partial record.
• Weighted sampling: outer while loop in do_weighted_sampling was dead code (the inner for consumes the iterator, no retry possible). Removed; single clear pass with the existing under-fill warning.
• Cluster sampling: pre-sized unique_clusters / all_clusters with the requested sample size instead of the cardinality, causing the HashMap to rehash its way up. Now uses cardinality (and falls back to the estimate when cardinality is unknown).
• parse_timestamp: a millisecond Unix timestamp like 1_704_067_200_000 parsed as seconds (year ~55,899). Disambiguated by magnitude (10^11 cutoff). Range-form check ((-SEC_LIMIT..SEC_LIMIT).contains(&ts_val)) so i64::MIN doesn't panic.
• do_reservoir_sampling: tightened bounds (reservoir.len().min(sample_size)) so an undersized reservoir slice is defensible. Removed unnecessary unsafe { get_unchecked_mut(...) } blocks here, in do_stratified_sampling, and in sample_indices.

Cleanups

• get_column_index: direct &[u8] byte comparison; no more String::from_utf8_lossy allocation per field.
• check_stats_cache: dropped the chars().all(char::is_numeric) Unicode-numeric pre-check — just parse::<usize>().ok().or_else(...). flag_force synced via debug_assert! instead of the misleading false literal.
• sample_stratified: collapsed the parallel strata_counts HashMap (its count was never read) and the parallel records_seen map into a single HashMap<Vec<u8>, (Vec<ByteRecord>, usize)>. Halves per-record lookups in the hot loop and removes the awkward "lookup-or-insert" dance.
• Time-series aggregate_records: numeric-column detection hoisted out of the per-interval loop. sample_timeseries does one O(records × cols) scan after sorting (with early exit once every column is ruled out). aggregate_records now takes a precomputed &[bool] mask and &[&ByteRecord] (no per-interval ByteRecord clones).
• with_rng! macro: replaces the recurring three-arm match rng_kind { Standard => …, Faster => …, Cryptosecure => … } blocks across sample_systematic, sample_stratified, sample_weighted, sample_cluster, sample_timeseries (random-offset branch), and the default INDEXED path in run(). Net: ~80 LOC deleted, "did I update all three branches?" trip-hazards removed, log lines unified to {:?} <METHOD> sampling….
• stream_bernoulli_sampling: lazy RNG creation (only the chosen kind, was eagerly building all three including slow-init HC128).
• Stale comment on do_reservoir_sampling ("constant memory" → O(k)).

Test plan

• cargo build --locked --bin qsv -F all_features — clean
• cargo t sample -F all_features — 73 / 73 pass
• cargo clippy --locked --bin qsv -F all_features -- -D warnings — clean
• (reviewer) sanity-check streaming Bernoulli against a remote URL with embedded quoted newlines in a header, and against --max-size mid-record truncation
• (reviewer) sanity-check qsv stats cache integration on stratified / weighted / cluster paths

🤖 Generated with Claude Code

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 106 complexity

Metric	Results
Complexity	106

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[Copilot]

Pull request overview

This PR refactors and tightens qsv sample by removing duplicated RNG dispatch code, improving performance in several sampling methods, and fixing multiple correctness issues—most notably around streaming Bernoulli sampling over remote URLs.

Changes:

• Fixes streaming Bernoulli sampling correctness (quoted-newline-safe record boundaries, --no-headers handling, early probability validation, --max-size enforcement) and reduces RNG init overhead.
• Simplifies sampling implementations (removes dead retry loop in weighted sampling, removes several unsafe indexing sites, bounds reservoir writes defensively).
• Performance/maintainability cleanups (byte-based column lookup, prealloc tweaks for cluster sampling, precomputed numeric-column mask for time-series aggregation, with_rng! macro to unify RNG branching).