Testing package#2032
Merged
Merged
Conversation
Collaborator
|
never mind the typos CI warning... |
jqnatividad
added a commit
that referenced
this pull request
May 11, 2026
Address roborev #2032: in the previous commit (d9fe03e), `argv_has_flag` and its doc comment were inserted *between* `can_enable_frequent_items`'s doc comment and the function body. Because Rust doc comments attach to the next item, the entire docstring block (including the `user_set_sketch_method` paragraph and the trailing "Returns false if any conflicting flag is set..." line) bound to `argv_has_flag`, leaving `can_enable_frequent_items` with no doc comment at all. Move `argv_has_flag` (with its own 4-line doc) to live *after* `can_enable_frequent_items`, mirroring the layout in stats.rs where the ordering was correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jqnatividad
added a commit
that referenced
this pull request
May 11, 2026
) * feat(stats,frequency): auto-enable DataSketches estimators on OOM When --memcheck is set and util::mem_file_check returns OutOfMemory, stats and frequency now auto-enable their DataSketches-backed estimators (t-digest + HyperLogLog for stats; Misra-Gries Frequent Items for frequency) in addition to the existing auto-index fallback. Conflict guards mirror the explicit-validation rejections so the auto-enable only flips methods that would have passed validation if set by hand. A wwarn! lists the auto-enabled estimators; the original OOM is only propagated when neither fallback engages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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> * address Copilot review: stream OOM fixture, drop drift-prone line refs, add -- prefix - tests/workdir.rs: rewrite build_large_oom_csv to stream rows directly to a csv::Writer instead of building a 10M-row Vec in memory first. Avoids ~1.5 GB of String allocation that would OOM the test harness itself on memory-constrained hosts, defeating the purpose of the ignored OOM tests. - src/cmd/stats.rs, src/cmd/frequency.rs: replace hard-coded intra-file line-number references in the try_enable_approx_sketches and can_enable_frequent_items doc-comments with descriptive references to the validator/dispatch blocks they mirror. - src/cmd/frequency.rs: add the missing -- prefix to "sketch-method frequent_items" in the --memcheck USAGE help text; regenerate docs/help/frequency.md and .claude/skills/qsv/qsv-frequency.json. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(stats,frequency): clarify OOM fallback fires in both NORMAL and CONSERVATIVE mode The OOM auto-fallback to DataSketches estimators is gated on the result of util::mem_file_check, NOT on whether --memcheck is set. The in-memory load check runs unconditionally on the non-parallel path; --memcheck only switches the check from NORMAL mode (vs. total memory) to the stricter CONSERVATIVE mode (vs. available + swap × platform factor). The fallback can therefore trigger without --memcheck too — just much less often, since NORMAL mode only trips when the file is larger than ~80% of total RAM. Rewrote --memcheck USAGE in stats.rs and frequency.rs to: - Lead with what --memcheck actually does (CONSERVATIVE vs. NORMAL). - Reference QSV_MEMORY_CHECK as the env-var equivalent. - Describe the OOM fallback as a behavior of the load check itself, not of --memcheck specifically. Updated CHANGELOG.md and docs/STATS_DEFINITIONS.md to match. Regenerated docs/help/{stats,frequency}.md and the corresponding MCP skill JSONs from the new USAGE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * address Copilot review: real opt-out for OOM auto-enable + assert command success in tests Two issues raised by Copilot on the OOM auto-fallback: (1) The wwarn promised users could re-run with explicit --quantile-method exact / --cardinality-method exact / --sketch-method exact to disable the auto-enable, but the code only checked the parsed flag value. docopt fills in the default "exact" either way, so an explicit --foo-method exact was indistinguishable from omitting the flag — making the documented opt-out a no-op. Fix: scan argv for the literal flag names ("--foo-method" or "--foo-method=...") to detect explicit user intent. Thread that through try_enable_approx_sketches / can_enable_frequent_items via new user_set_* parameters; the auto-enable is suppressed when the user explicitly provided the flag (regardless of value). Documented in STATS_DEFINITIONS.md. (2) The new OOM tests used wrk.output_stderr, which returns stderr regardless of exit status — a command that errored out after printing the auto-enable wwarn would still pass the test. Fix: add a wrk.stderr_on_success helper that asserts status.success() before returning stderr (with the same diagnostic-rich panic format as assert_success). Migrate the 6 new stats/frequency OOM tests to use it. Other call sites of output_stderr left untouched — they test failure paths where non-success is intentional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(frequency): restore can_enable_frequent_items doc comment Address roborev #2032: in the previous commit (d9fe03e), `argv_has_flag` and its doc comment were inserted *between* `can_enable_frequent_items`'s doc comment and the function body. Because Rust doc comments attach to the next item, the entire docstring block (including the `user_set_sketch_method` paragraph and the trailing "Returns false if any conflicting flag is set..." line) bound to `argv_has_flag`, leaving `can_enable_frequent_items` with no doc comment at all. Move `argv_has_flag` (with its own 4-line doc) to live *after* `can_enable_frequent_items`, mirroring the layout in stats.rs where the ordering was correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Testing deb package