fix: aggregation performance#6425
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 6674336
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughGrouped row construction and aggregation are optimized through unique-row normalization, cached grouping values, single-pass range aggregators, and lazy aggregation caches. Benchmark documentation and the bundle-size limit are also updated. ChangesGrouped aggregation performance
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 2
🤖 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 `@package.json`:
- Line 61: Revert the package size limit from 25 KB to 20 KB so the configured
constraint matches the documented bundle size and enforces the existing strict
threshold.
In `@packages/table-core/src/features/column-grouping/createGroupedRowModel.ts`:
- Around line 231-246: Update the grouping value logic around
_groupingValuesCache so that when getGroupingValue exists but the cache is
absent, it still computes groupingValue by calling getGroupingValue with
row.original, row.index, and row. Preserve the existing cached-value lookup and
cache-population paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc33888f-8dfc-46a6-b33c-f9c892857b08
📒 Files selected for processing (7)
package.jsonpackages/table-core/src/features/column-grouping/createGroupedRowModel.tspackages/table-core/src/features/row-aggregation/aggregationFns.tspackages/table-core/src/features/row-aggregation/rowAggregationFeature.tspackages/table-core/src/features/row-aggregation/rowAggregationFeature.types.tspackages/table-core/src/features/row-aggregation/rowAggregationFeature.utils.tsperf-agg.md
Comment on lines
+231
to
+246
| if (getGroupingValue) { | ||
| const cache = (row as any)._groupingValuesCache as | ||
| | Record<string, unknown> | ||
| | undefined | ||
| if (cache && hasOwn(cache, columnId)) { | ||
| groupingValue = cache[columnId] | ||
| } else if (cache) { | ||
| groupingValue = cache[columnId] = getGroupingValue( | ||
| row.original, | ||
| row.index, | ||
| row, | ||
| ) | ||
| } | ||
| } else { | ||
| groupingValue = row.getValue(columnId) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provide a fallback when _groupingValuesCache is undefined.
If cache evaluates to falsy, groupingValue is never assigned and remains undefined. Even though the cache might currently be eagerly initialized (as noted in perf-agg.md), relying on this behavior makes the code brittle against future refactoring (such as lazy initialization). Add an else block to compute the grouping value when the cache is absent.
🐛 Proposed fix
if (getGroupingValue) {
const cache = (row as any)._groupingValuesCache as
| Record<string, unknown>
| undefined
if (cache && hasOwn(cache, columnId)) {
groupingValue = cache[columnId]
- } else if (cache) {
- groupingValue = cache[columnId] = getGroupingValue(
+ } else {
+ groupingValue = getGroupingValue(
row.original,
row.index,
row,
)
+ if (cache) {
+ cache[columnId] = groupingValue
+ }
}
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (getGroupingValue) { | |
| const cache = (row as any)._groupingValuesCache as | |
| | Record<string, unknown> | |
| | undefined | |
| if (cache && hasOwn(cache, columnId)) { | |
| groupingValue = cache[columnId] | |
| } else if (cache) { | |
| groupingValue = cache[columnId] = getGroupingValue( | |
| row.original, | |
| row.index, | |
| row, | |
| ) | |
| } | |
| } else { | |
| groupingValue = row.getValue(columnId) | |
| } | |
| if (getGroupingValue) { | |
| const cache = (row as any)._groupingValuesCache as | |
| | Record<string, unknown> | |
| | undefined | |
| if (cache && hasOwn(cache, columnId)) { | |
| groupingValue = cache[columnId] | |
| } else { | |
| groupingValue = getGroupingValue( | |
| row.original, | |
| row.index, | |
| row, | |
| ) | |
| if (cache) { | |
| cache[columnId] = groupingValue | |
| } | |
| } | |
| } else { | |
| groupingValue = row.getValue(columnId) | |
| } |
🤖 Prompt for 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.
In `@packages/table-core/src/features/column-grouping/createGroupedRowModel.ts`
around lines 231 - 246, Update the grouping value logic around
_groupingValuesCache so that when getGroupingValue exists but the cache is
absent, it still computes groupingValue by calling getGroupingValue with
row.original, row.index, and row. Preserve the existing cached-value lookup and
cache-population paths.
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
1 participant
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.
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
Performance
Bug Fixes
Documentation