feat(clp-s): Support per-archive unique-value aggregation output to stdout and the results cache.#2367
Open
davemarco wants to merge 66 commits into
Open
feat(clp-s): Support per-archive unique-value aggregation output to stdout and the results cache.#2367davemarco wants to merge 66 commits into
davemarco wants to merge 66 commits into
Conversation
…e results cache. Allows --count and --count-by-time to be used with the results-cache output handler, writing aggregation results directly to MongoDB via _id-keyed atomic $inc upserts so that partial results from multiple search processes merge correctly without a reducer.
…he search usage text outputs to the reducer.
…nation and simplify comments.
…dout. Adds `--count` and `--count-by-time` to the stdout output handler, emitting per-archive results as newline-delimited JSON. The options may be given without naming the `stdout` handler (e.g. `clp-s s <archives> <query> --count`), and the kv-ir search path now reports these aggregations as unsupported, consistent with the reducer and results-cache handlers.
… results cache. Adds --min/--max <field> aggregations, extracting the field's numeric value from each matched record's marshalled JSON. Unifies the results-cache count handlers into a single AggregationToResultsCacheOutputHandler handling count, count-by-time, min, and max.
…ers. - Take string_view for the results-cache handler uri/collection/dataset/archive_id params and materialize std::string only at the mongocxx call sites. - Drop the redundant ::clp_s:: qualifier on the count handlers' base class. - Rename the count-by-time write timestamp param to timestamp_ms to match the bucket size unit.
…n handler. Merges the results-cache count work and applies the same review fixes to AggregationToStdoutOutputHandler: take string_view for archive_id, drop the redundant ::clp_s:: qualifier on the base class, rename the count-by-time bucket size to count_by_time_bucket_size_ms, and rename the count-by-time write timestamp param to timestamp_ms.
…e results cache.
Add a --unique FIELD aggregation that collects the distinct scalar values of a field across matched records, emitting one {field, value} result document per value through the existing aggregation sinks. Factor the shared field-path tokenization and JSON navigation out of MinMaxAggregation into reusable helpers, and rewrite the reducer output-handler restriction as a whitelist.
Contributor
WalkthroughAdds ChangesUnique aggregation and results-cache output
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SearchCLI
participant UniqueAggregator
participant ResultsCacheSink
participant ResultsCache
SearchCLI->>UniqueAggregator: construct from --unique field
SearchCLI->>UniqueAggregator: add_record for matched records
UniqueAggregator-->>ResultsCacheSink: emit distinct aggregation results
ResultsCacheSink->>ResultsCache: batch insert_many
ResultsCache-->>ResultsCacheSink: flush result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add bool to AggregationValue so --unique treats booleans as distinct scalar values, and flush ResultsCacheSink in batches so high-cardinality aggregations don't buffer every document, surfacing mid-stream database errors.
davemarco
changed the title
davemarco
changed the title
- Rename the per-record aggregator classes and their variant from `*Aggregation`/`Aggregation` to `*Aggregator`/`Aggregator`, keeping "aggregation" for the operation's data/output (value, result, sink, output handler). - Rename ambiguous `_ms` identifiers to `_millisecs`. - Use `<clp_s/...>` includes, brace-init constexpr members, and tidy doc-comments (possessive wording, hoist implementation notes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… category comments.
3 tasks
# Conflicts: # components/core/src/clp_s/AggregationSink.cpp # components/core/src/clp_s/AggregationSink.hpp # components/core/src/clp_s/CommandLineArguments.cpp # components/core/src/clp_s/CommandLineArguments.hpp # components/core/src/clp_s/aggregators.cpp # components/core/src/clp_s/aggregators.hpp # components/core/src/clp_s/archive_constants.hpp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/src/clp_s/aggregators.cpp`:
- Around line 168-195: Add or confirm unit coverage for UniqueAggregator,
including scalar conversion through to_aggregation_value (especially booleans),
field lookup through find_field_value, and deduplication of mixed-type values
across heterogeneous schemas. If tests are maintained elsewhere in the PR,
ensure they explicitly exercise these behaviors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a8aaab0-89fd-4bcc-a944-c2ce5b6a104b
📒 Files selected for processing (8)
components/core/src/clp_s/AggregationSink.cppcomponents/core/src/clp_s/AggregationSink.hppcomponents/core/src/clp_s/CommandLineArguments.cppcomponents/core/src/clp_s/CommandLineArguments.hppcomponents/core/src/clp_s/aggregators.cppcomponents/core/src/clp_s/aggregators.hppcomponents/core/src/clp_s/archive_constants.hppcomponents/core/src/clp_s/clp-s.cpp
davemarco
changed the title
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.
Description
Adds a --unique search aggregation, computing the distinct values of a field across matched records and writing them to stdout or the results cache. Continues the aggregation work from #2326, #2342, and #2345.
Outputs one document per distinct value, for both destinations:
{archive_id, field, value}
Changes
Checklist
breaking change.
Validation performed
Tested a few queries on mongodb dataset
Summary by CodeRabbit
New Features
--uniquesearch aggregation option to return distinct values for a specified field.--uniquesupports integer, floating-point, string, and boolean values.--unique.Performance
Bug Fixes
--uniquefield names and field paths (including rejecting unescaped wildcards).