feat(pivotp): add group-by mode (closes #3697)#3698
Merged
Conversation
Support a group-by mode when <on-cols> is omitted: updated USAGE/help text to document PIVOT vs GROUP-BY modes, made positional args optional (arg_on_cols/arg_input -> Option), and resolved positional parsing to detect mode. In group-by mode, --index is required, --agg defaults to count/len (smart -> len), and --values can be omitted to produce a single count column; several pivot-only flags (--subtotal, --validate, --sort-columns) are rejected in group-by mode. Implemented separate execution paths: group-by uses group_by/group_by_stable with appropriate aggregation expressions, while pivot retains the existing pivot logic (on_columns discovery, validation, order restoration, and optional column sorting). Also adjusted input path handling for LazyCsvReader and pschema lookup to use the resolved input string.
…rden dispatch - Reject `--agg none` during group-by validation instead of silently falling through to count - Remove dead if/else branch where both arms returned "smart" - Replace catch-all `_ =>` in group-by agg match with explicit `"len" | "smart"` arm and `unreachable!()` to prevent silent divergence from pivot-mode dispatch - Add cross-reference comments between the two aggregation match blocks - Add safety comments explaining why unwrap() calls in pivot mode are sound - Fix USAGE text: --agg smart resolves to len (count), not defaults to len - Add test for --agg none rejection in group-by mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…refs Move the `--agg none` group-by validation after `to_lowercase()` so that `--agg None` and `--agg NONE` are also rejected with a friendly error instead of hitting `unreachable!()` and panicking. Replace fragile `~line NNN` cross-references in comments with landmark-based descriptions that won't drift with edits. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 20 |
TIP This summary will be updated as you push new changes. Give us feedback
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new “group-by mode” to qsv pivotp when <on-cols> is omitted, enabling simple group_by + aggregate workflows (e.g., counts per group) while keeping existing pivot behavior intact.
Changes:
- Updated
pivotpCLI/arg parsing and help text to support both pivot mode (<on-cols> <input>) and group-by mode (<input>). - Implemented a Polars group-by execution path (including support for
--grand-totaland--maintain-order) and mode-specific flag validation. - Added a new suite of group-by mode tests, including success cases and error cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/cmd/pivotp.rs |
Adds group-by mode execution path, positional-arg resolution, mode-specific validations, and updated CLI documentation. |
tests/test_pivotp.rs |
Adds new tests covering group-by mode behavior (counts, sums, multi-index/values, totals) and error scenarios. |
…ests - Use len() instead of count() in group-by agg expressions for null- consistency with pivot mode's Expr::Element.len() - Reject --agg item in group-by mode with clear error (no single-value semantics equivalent) - Update --maintain-order help text to describe both modes - Skip index_order computation when --maintain-order is set (rely on group_by_stable); keep lazy pipeline for non-maintain-order case - Assert stderr error messages in all error-case tests - Add --agg item error test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use mode-aware "Invalid {group-by|pivot} aggregation function" error
- Clarify --col-separator is ignored in group-by mode
- Clarify --grand-total sums numeric non-index columns
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
resolves #3697 |
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
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.
Summary
pivotp— when<on-cols>is omitted, performs a Polarsgroup_by+ aggregate instead of a pivotqsv pivotp --index GROUP --agg len data.csv)--grand-total,--maintain-order, and all aggregation functions work in group-by mode--subtotal,--validate,--sort-columns) with clear error messagesExample (from issue #3697)
Test plan
cargo clippycleanCloses #3697
🤖 Generated with Claude Code