fix(stats,frequency): address roborev #2028 findings

- Dedupe build_large_oom_csv into tests/workdir.rs so test_stats and
  test_frequency share one source of truth (Low #1).
- Document the pre-indexed + OOM → sketch fallback path in --memcheck
  USAGE text, CHANGELOG, and docs/STATS_DEFINITIONS.md (Low #2).
- Drop the dead flag_sketch_method='frequent_items' assignment before
  run_frequent_items — confirmed run_frequent_items does not consult
  flag_sketch_method (Low #3).
- Tighten the stats and frequency OOM wwarn messages to "Re-run with
  explicit ... exact to disable the auto-enable" — matches the
  established frequency wording and removes the misleading "override"
  phrasing (Low #4).

Verified Low #5 separately: which_stats() already gates mad on
!approx_quantiles regardless of flag_everything/flag_mad, so the
auto-disable promised by the wwarn is honored.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: jqnatividad

.claude/skills/qsv/qsv-frequency.json

@@ -77,7 +77,7 @@
       {
         "flag": "--memcheck",
         "type": "flag",
-        "description": "Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. On OOM, qsv auto-creates an index when possible and also switches to the Frequent Items sketch (Apache DataSketches Misra-Gries, equivalent to sketch-method frequent_items) where compatible, before failing. A wwarn is emitted when the sketch fallback engages."
+        "description": "Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. On OOM, qsv auto-creates an index when no index exists (skipped for stdin) and ALSO switches to the Frequent Items sketch (Apache DataSketches Misra-Gries, equivalent to sketch-method frequent_items) where compatible. The sketch fallback can also fire when an index is already present and the OOM still trips (e.g., when jobs is pinned to 1 on a pre-indexed file). A wwarn is emitted when the sketch fallback engages."
       },
       {
         "flag": "--no-float",

.claude/skills/qsv/qsv-stats.json

@@ -81,7 +81,7 @@
       {
         "flag": "--memcheck",
         "type": "flag",
-        "description": "Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. This option is ignored when computing default, streaming statistics, as it is not needed. On OOM, qsv auto-creates an index when possible and also switches to approx quantile + approx cardinality methods (DataSketches t-digest and HyperLogLog) where compatible, before failing. A wwarn is emitted listing the auto-enabled estimators."
+        "description": "Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. This option is ignored when computing default, streaming statistics, as it is not needed. On OOM, qsv auto-creates an index when no index exists (skipped for stdin) and ALSO switches to approx quantile + approx cardinality methods (DataSketches t-digest and HyperLogLog) where compatible. The sketch fallback can also fire when an index is already present and the OOM still trips (e.g., when jobs is pinned to 1 on a pre-indexed file). A wwarn is emitted listing the auto-enabled estimators."
       },
       {
         "flag": "--mode",

CHANGELOG.md

@@ -39,7 +39,7 @@ Detailed MCP Server and Cowork Plugin changes are documented in the MCP Server/C
 - `exclude`: add stdin support and memcheck [#3749](https://github.com/dathere/qsv/pull/3749)
 
 ### Changed
-- `stats` / `frequency`: when `--memcheck` is set and `util::mem_file_check` returns OOM, qsv now auto-enables the Apache DataSketches-backed estimators (t-digest + HyperLogLog for `stats`; Misra-Gries Frequent Items for `frequency`) in addition to the existing auto-index fallback, where flag conflicts allow. The OOM error is only propagated when neither fallback engages. A `wwarn!` is emitted listing the auto-enabled estimators. Explicit `--quantile-method exact` / `--cardinality-method exact` / `--sketch-method exact` still suppresses the auto-enable for that method.
+- `stats` / `frequency`: when `--memcheck` is set and `util::mem_file_check` returns OOM, qsv now auto-enables the Apache DataSketches-backed estimators (t-digest + HyperLogLog for `stats`; Misra-Gries Frequent Items for `frequency`) in addition to the existing auto-index fallback, where flag conflicts allow. The OOM error is only propagated when neither fallback engages. A `wwarn!` is emitted listing the auto-enabled estimators. The sketch fallback can fire even when an index is already present (e.g., with `--jobs 1` on a pre-indexed file) — this is a behavior change from the previous "error out" path in that narrow case. Explicit `--quantile-method exact` / `--cardinality-method exact` / `--sketch-method exact` still suppresses the auto-enable for that method.
 - **BREAKING** `excel`: `--metadata csv` column ordering for `type`, `visible`, and `headers` is corrected. Previously the CSV header row declared `type, visible, headers` but the data rows pushed values in the order `headers, typ, visible`, so under each named column the wrong values appeared (the `type` column held the headers list, `visible` held the type, and `headers` held the visibility). The CSV output now matches the `--metadata json` (`SheetMetadata` struct) field order: `index, sheet_name, type, visible, headers, column_count, …`. Pipelines that consumed `qsv excel --metadata csv` and indexed by column position must shift those three columns; consumers that indexed by header name see corrected values automatically.
 - **BREAKING** `enum`: `--hash` digest values change. The hashed input now carries a `u64` length prefix per field (to fix the multi-column collision bug above), so every `--hash` digest differs from earlier qsv versions — single-column hashes change identity values too, and stored hashes from earlier qsv versions will not match. Same input still hashes deterministically across rows and runs in ≥ this version.
 - **BREAKING** `luau`: `qsv_loadcsv` now returns the headers table 1-indexed (per Lua convention). Scripts that accessed `headers[0]` or iterated `for i = 0, #headers - 1` must shift to `headers[1]` and `for i = 1, #headers` (or `ipairs(headers)`). Previously `headers[1]` returned the *second* header.

docs/STATS_DEFINITIONS.md

@@ -363,7 +363,7 @@ By default, `stats` produces **exact, deterministic** results. Three opt-in flag
    - `--quantile-method` auto-enables unless `--weight` is set; if `--mad` or `--everything` is also set, MAD is auto-disabled (mirroring the existing `--quantile-method approx` guard).
    - `--cardinality-method` auto-enables unless `--infer-boolean` is set.
 
-A `wwarn!` is emitted listing each auto-enabled estimator. The original OOM error is only propagated when **neither** fallback engages. Users can override by passing `--quantile-method exact` or `--cardinality-method exact` explicitly (the auto-enable only flips fields that were left at their `exact` default).
+A `wwarn!` is emitted listing each auto-enabled estimator. The original OOM error is only propagated when **neither** fallback engages. The sketch fallback can fire even when an index is already present and the OOM check still trips (e.g., with `--jobs 1` on a pre-indexed file) — that is a behavior change from the previous "error out" path in this narrow case. Users can disable the auto-enable by passing `--quantile-method exact` or `--cardinality-method exact` explicitly (the auto-enable only flips fields that were left at their `exact` default).
 
 **See also:** [t-digest paper (Dunning, 2019)](https://arxiv.org/abs/1902.04023), [HyperLogLog (Flajolet et al., 2007)](https://en.wikipedia.org/wiki/HyperLogLog), [Apache DataSketches](https://datasketches.apache.org/).
 

docs/help/frequency.md

@@ -155,7 +155,7 @@ qsv frequency --help
 | &nbsp;`‑o,`<br>`‑‑output`&nbsp; | string | Write output to <file> instead of stdout. |  |
 | &nbsp;`‑n,`<br>`‑‑no‑headers`&nbsp; | flag | When set, the first row will NOT be included in the frequency table. Additionally, the 'field' column will be 1-based indices instead of header names. |  |
 | &nbsp;`‑d,`<br>`‑‑delimiter`&nbsp; | string | The field delimiter for reading CSV data. Must be a single character. (default: ,) |  |
-| &nbsp;`‑‑memcheck`&nbsp; | flag | Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. On OOM, qsv auto-creates an index when possible and also switches to the Frequent Items sketch (Apache DataSketches Misra-Gries, equivalent to sketch-method frequent_items) where compatible, before failing. A wwarn is emitted when the sketch fallback engages. |  |
+| &nbsp;`‑‑memcheck`&nbsp; | flag | Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. On OOM, qsv auto-creates an index when no index exists (skipped for stdin) and ALSO switches to the Frequent Items sketch (Apache DataSketches Misra-Gries, equivalent to sketch-method frequent_items) where compatible. The sketch fallback can also fire when an index is already present and the OOM still trips (e.g., when jobs is pinned to 1 on a pre-indexed file). A wwarn is emitted when the sketch fallback engages. |  |
 
 ---
 **Source:** [`src/cmd/frequency.rs`](https://github.com/dathere/qsv/blob/master/src/cmd/frequency.rs)

docs/help/stats.md

@@ -282,7 +282,7 @@ qsv stats --help
 | &nbsp;`‑o,`<br>`‑‑output`&nbsp; | string | Write output to <file> instead of stdout. |  |
 | &nbsp;`‑n,`<br>`‑‑no‑headers`&nbsp; | flag | When set, the first row will NOT be interpreted as column names. i.e., They will be included in statistics. |  |
 | &nbsp;`‑d,`<br>`‑‑delimiter`&nbsp; | string | The field delimiter for READING CSV data. Must be a single character. (default: ,) |  |
-| &nbsp;`‑‑memcheck`&nbsp; | flag | Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. This option is ignored when computing default, streaming statistics, as it is not needed. On OOM, qsv auto-creates an index when possible and also switches to approx quantile + approx cardinality methods (DataSketches t-digest and HyperLogLog) where compatible, before failing. A wwarn is emitted listing the auto-enabled estimators. |  |
+| &nbsp;`‑‑memcheck`&nbsp; | flag | Check if there is enough memory to load the entire CSV into memory using CONSERVATIVE heuristics. This option is ignored when computing default, streaming statistics, as it is not needed. On OOM, qsv auto-creates an index when no index exists (skipped for stdin) and ALSO switches to approx quantile + approx cardinality methods (DataSketches t-digest and HyperLogLog) where compatible. The sketch fallback can also fire when an index is already present and the OOM still trips (e.g., when jobs is pinned to 1 on a pre-indexed file). A wwarn is emitted listing the auto-enabled estimators. |  |
 
 ---
 **Source:** [`src/cmd/stats.rs`](https://github.com/dathere/qsv/blob/master/src/cmd/stats.rs)

src/cmd/frequency.rs

@@ -263,12 +263,15 @@ Common options:
                            Must be a single character. (default: ,)
     --memcheck             Check if there is enough memory to load the entire
                            CSV into memory using CONSERVATIVE heuristics.
-                           On OOM, qsv auto-creates an index when possible and
-                           also switches to the Frequent Items sketch
-                           (Apache DataSketches Misra-Gries, equivalent to
-                           sketch-method frequent_items) where compatible,
-                           before failing. A wwarn is emitted when the sketch
-                           fallback engages.
+                           On OOM, qsv auto-creates an index when no index
+                           exists (skipped for stdin) and ALSO switches to the
+                           Frequent Items sketch (Apache DataSketches
+                           Misra-Gries, equivalent to sketch-method
+                           frequent_items) where compatible. The sketch
+                           fallback can also fire when an index is already
+                           present and the OOM still trips (e.g., when jobs is
+                           pinned to 1 on a pre-indexed file). A wwarn is
+                           emitted when the sketch fallback engages.
 "#;
 
 use core::hint::cold_path;
@@ -852,14 +855,16 @@ pub fn run(argv: &[&str]) -> CliResult<()> {
                 // Sketch fallback: only for OOM (not other CliErrors).
                 // The Frequent Items dispatch at frequency.rs:787 normally fires before
                 // mem_file_check runs, so we re-route into run_frequent_items() directly
-                // from here rather than just flipping the flag and falling through.
+                // here rather than mutating flag_sketch_method and falling through —
+                // run_frequent_items doesn't consult flag_sketch_method, so the field
+                // stays at "exact" and that's fine (cache key / diagnostics use other
+                // paths). Verified via grep over run_frequent_items's body.
                 if matches!(e, crate::CliError::OutOfMemory(_)) && can_enable_frequent_items(&args)
                 {
-                    args.flag_sketch_method = "frequent_items".to_string();
                     wwarn!(
                         "OOM during memory check: auto-enabling --sketch-method frequent_items \
                          (Misra-Gries, map size {n}). Re-run with explicit --sketch-method exact \
-                         to override.",
+                         to disable the auto-enable.",
                         n = args.flag_sketch_map_size,
                     );
                     return args.run_frequent_items(&rconfig);

src/cmd/stats.rs

@@ -355,11 +355,15 @@ Common options:
                            CSV into memory using CONSERVATIVE heuristics.
                            This option is ignored when computing default, streaming
                            statistics, as it is not needed.
-                           On OOM, qsv auto-creates an index when possible and
-                           also switches to approx quantile + approx cardinality
-                           methods (DataSketches t-digest and HyperLogLog) where
-                           compatible, before failing. A wwarn is emitted listing
-                           the auto-enabled estimators.
+                           On OOM, qsv auto-creates an index when no index
+                           exists (skipped for stdin) and ALSO switches to
+                           approx quantile + approx cardinality methods
+                           (DataSketches t-digest and HyperLogLog) where
+                           compatible. The sketch fallback can also fire when
+                           an index is already present and the OOM still trips
+                           (e.g., when jobs is pinned to 1 on a pre-indexed
+                           file). A wwarn is emitted listing the auto-enabled
+                           estimators.
 "#;
 
 /*
@@ -1542,8 +1546,9 @@ pub fn run(argv: &[&str]) -> CliResult<()> {
                             if !enabled.is_empty() {
                                 wwarn!(
                                     "OOM during memory check: auto-enabling DataSketches \
-                                     estimators ({}). Re-run with explicit \
-                                     --quantile-method/--cardinality-method to override.",
+                                     estimators ({}). Re-run with explicit --quantile-method \
+                                     exact / --cardinality-method exact to disable the \
+                                     auto-enable.",
                                     enabled.join(", ")
                                 );
                             } else if !index_succeeded {

tests/test_frequency.rs

@@ -6721,28 +6721,16 @@ fn frequency_sketch_method_frequent_items_with_null_sorted_is_rejected() {
 // in addition to the existing auto-index fallback. These tests need a
 // multi-GB file to deterministically trigger OOM and are #[ignore]'d by
 // default; run with `--ignored` to exercise them.
-
-fn build_large_oom_csv(name: &str) -> (Workdir, std::path::PathBuf) {
-    let wrk = Workdir::new(name);
-    let mut data = vec![svec!["col1", "col2", "col3", "col4", "col5"]];
-    for i in 0..10_000_000 {
-        data.push(vec![
-            format!("value_{}_with_some_padding_to_make_it_larger", i),
-            format!("another_value_{}_with_more_data", i),
-            format!("data_{}", i),
-            format!("field_{}_content", i),
-            format!("final_field_{}_with_additional_text", i),
-        ]);
-    }
-    let path = wrk.path("large_data.csv");
-    wrk.create("large_data.csv", data);
-    (wrk, path)
-}
+//
+// The shared `build_large_oom_csv` helper lives in `tests/workdir.rs` so the
+// row count, column shape, and padding stay in sync with the parallel stats
+// tests; see `stats_oom_*` in tests/test_stats.rs.
 
 #[test]
 #[ignore = "Requires a multi-GB file to trigger OOM via mem_file_check"]
 fn frequency_oom_auto_enables_frequent_items() {
-    let (wrk, test_file) = build_large_oom_csv("frequency_oom_auto_enables_frequent_items");
+    let (wrk, test_file) =
+        crate::workdir::build_large_oom_csv("frequency_oom_auto_enables_frequent_items");
     let mut cmd = wrk.command("frequency");
     cmd.arg("--memcheck")
         .env("QSV_FREEMEMORY_HEADROOM_PCT", "90")
@@ -6761,7 +6749,7 @@ fn frequency_oom_auto_enables_frequent_items() {
 #[test]
 #[ignore = "Requires a multi-GB file to trigger OOM via mem_file_check"]
 fn frequency_oom_skips_when_asc_set() {
-    let (wrk, test_file) = build_large_oom_csv("frequency_oom_skips_when_asc_set");
+    let (wrk, test_file) = crate::workdir::build_large_oom_csv("frequency_oom_skips_when_asc_set");
     let mut cmd = wrk.command("frequency");
     cmd.arg("--memcheck")
         .arg("--asc")
@@ -6777,7 +6765,8 @@ fn frequency_oom_skips_when_asc_set() {
 #[test]
 #[ignore = "Requires a multi-GB file to trigger OOM via mem_file_check"]
 fn frequency_oom_skips_when_json_output() {
-    let (wrk, test_file) = build_large_oom_csv("frequency_oom_skips_when_json_output");
+    let (wrk, test_file) =
+        crate::workdir::build_large_oom_csv("frequency_oom_skips_when_json_output");
     let mut cmd = wrk.command("frequency");
     cmd.arg("--memcheck")
         .arg("--json")

tests/test_stats.rs

@@ -4206,28 +4206,16 @@ fn stats_auto_index_creation_on_oom() {
 // the existing auto-index fallback. Like stats_auto_index_creation_on_oom
 // above, these need a multi-GB file to deterministically trigger OOM, so
 // they are #[ignore]'d by default and must be run with `--ignored`.
-
-fn build_large_oom_csv(name: &str) -> (Workdir, std::path::PathBuf) {
-    let wrk = Workdir::new(name);
-    let mut data = vec![svec!["col1", "col2", "col3", "col4", "col5"]];
-    for i in 0..10_000_000 {
-        data.push(vec![
-            format!("value_{}_with_some_padding_to_make_it_larger", i),
-            format!("another_value_{}_with_more_data", i),
-            format!("data_{}", i),
-            format!("field_{}_content", i),
-            format!("final_field_{}_with_additional_text", i),
-        ]);
-    }
-    let path = wrk.path("large_data.csv");
-    wrk.create("large_data.csv", data);
-    (wrk, path)
-}
+//
+// The shared `build_large_oom_csv` helper lives in `tests/workdir.rs` so the
+// row count, column shape, and padding stay in sync with the parallel
+// frequency tests; see `frequency_oom_*` in tests/test_frequency.rs.
 
 #[test]
 #[ignore = "Requires a multi-GB file to trigger OOM via mem_file_check"]
 fn stats_oom_auto_enables_approx_sketches() {
-    let (wrk, test_file) = build_large_oom_csv("stats_oom_auto_enables_approx_sketches");
+    let (wrk, test_file) =
+        crate::workdir::build_large_oom_csv("stats_oom_auto_enables_approx_sketches");
     let mut cmd = wrk.command("stats");
     cmd.arg("--everything")
         .arg("--memcheck")
@@ -4251,7 +4239,8 @@ fn stats_oom_auto_enables_approx_sketches() {
 #[test]
 #[ignore = "Requires a multi-GB file to trigger OOM via mem_file_check"]
 fn stats_oom_skips_approx_quantiles_with_weight() {
-    let (wrk, test_file) = build_large_oom_csv("stats_oom_skips_approx_quantiles_with_weight");
+    let (wrk, test_file) =
+        crate::workdir::build_large_oom_csv("stats_oom_skips_approx_quantiles_with_weight");
     // Weight column is col1 (all distinct strings — qsv will treat parse failures
     // as default weight 1.0; this is acceptable for the test because we only care
     // that --weight is *set*, which is what suppresses the quantile auto-enable).
@@ -4276,8 +4265,9 @@ fn stats_oom_skips_approx_quantiles_with_weight() {
 #[test]
 #[ignore = "Requires a multi-GB file to trigger OOM via mem_file_check"]
 fn stats_oom_skips_approx_cardinality_with_infer_boolean() {
-    let (wrk, test_file) =
-        build_large_oom_csv("stats_oom_skips_approx_cardinality_with_infer_boolean");
+    let (wrk, test_file) = crate::workdir::build_large_oom_csv(
+        "stats_oom_skips_approx_cardinality_with_infer_boolean",
+    );
     let mut cmd = wrk.command("stats");
     cmd.arg("--everything")
         .arg("--memcheck")