PERF: Fast-path COUNT_VALID groupby for no-null inputs - #23756
PERF: Fast-path COUNT_VALID groupby for no-null inputs#23756fallintoplace wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change updates groupby count paths to detect actual nulls, adds no-null-mask tests, and introduces a pre-sorted count-scan benchmark with configurable row counts and validity modes. ChangesGroupby count scan behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR adds a localized fast path for COUNT_VALID on inputs without nulls and includes targeted test coverage; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Can you post the benchmark results? |
| test_single_scan(keys, vals, expect_keys, expect_vals, std::move(agg2)); | ||
| } | ||
|
|
||
| TYPED_TEST(groupby_count_scan_test, all_valid_values) |
There was a problem hiding this comment.
I'm not sure this warrants a typed-test. Can we reduce this to a single type so we do not generate 20 of these?
3780491 to
9008c17
Compare
Description
When there are no nulls, COUNT_VALID has the same input values as COUNT_ALL. The current code still creates a device column view and checks validity for every row.
Benchmark
Added
pre_sorted_count_valid_scanincpp/benchmarks/groupby/group_scan.cpp.Testing
Checklist