Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"size-limit": [
{
"path": "packages/table-core/dist/index.js",
"limit": "20 KB"
"limit": "25 KB"
Comment thread
KevinVandy marked this conversation as resolved.
}
],
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import { table_autoResetExpanded } from '../row-expanding/rowExpandingFeature.ut
import { table_autoResetPageIndex } from '../row-pagination/rowPaginationFeature.utils'
import {
aggregateColumnValue,
normalizeAggregationRows,
normalizeUniqueAggregationRows,
} from '../row-aggregation/rowAggregationFeature.utils'
import { row_getGroupingValue } from './columnGroupingFeature.utils'
import type { Row_ColumnGrouping } from './columnGroupingFeature.types'
import type { Column_Internal } from '../../types/Column'
import type { TableFeatures } from '../../types/TableFeatures'
import type { RowModel } from '../../core/row-models/coreRowModelsFeature.types'
import type { Table, Table_Internal } from '../../types/Table'
Expand Down Expand Up @@ -103,7 +103,7 @@ function _createGroupedRowModel<
const columnId = existingGrouping[depth] as string

// Group the rows together for this level
const rowGroupsMap = groupBy(rows, columnId)
const rowGroupsMap = groupBy(table, rows, columnId)

// Perform aggregations for each group
const aggregatedGroupedRows = Array.from(rowGroupsMap.entries()).map(
Expand All @@ -118,7 +118,13 @@ function _createGroupedRowModel<
subRow.parentId = id
})

const leafRows = normalizeAggregationRows(groupedRows, Infinity)
// Rows produced by groupBy are disjoint members of the pre-grouped
// row tree, so the duplicate-id guard is unnecessary; flat groups
// reuse the partition array as-is.
const leafRows = normalizeUniqueAggregationRows(
groupedRows,
Infinity,
) as Array<Row<TFeatures, TData>>

const row = constructRow(
table,
Expand Down Expand Up @@ -171,6 +177,7 @@ function _createGroupedRowModel<
column,
groupingRow: row,
rows: groupedRows,
uniqueRows: true,
})
return cache[colId]
},
Expand Down Expand Up @@ -203,19 +210,49 @@ function _createGroupedRowModel<
}

function groupBy<TFeatures extends TableFeatures, TData extends RowData = any>(
table: Table_Internal<TFeatures, TData>,
rows: Array<Row<TFeatures, TData>>,
columnId: string,
) {
const groupMap = new Map<any, Array<Row<TFeatures, TData>>>()

return rows.reduce((map, row) => {
const resKey = `${row_getGroupingValue(row, columnId)}`
const previous = map.get(resKey)
// Resolve the column once instead of per row: `table.getColumn` goes
// through the memoized-API dispatcher, which is far too expensive to sit
// inside this per-row loop. The branches below mirror
// `row_getGroupingValue`'s caching contract exactly.
const column = table_getColumn(table, columnId) as
| Column_Internal<TFeatures, TData, unknown>
| undefined
const getGroupingValue = column?.columnDef.getGroupingValue

for (let i = 0; i < rows.length; i++) {
const row = rows[i]!
let groupingValue
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)
}
Comment on lines +231 to +246

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


const resKey = `${groupingValue}`
const previous = groupMap.get(resKey)
if (!previous) {
map.set(resKey, [row])
groupMap.set(resKey, [row])
} else {
previous.push(row)
}
return map
}, groupMap)
}

return groupMap
}
122 changes: 80 additions & 42 deletions packages/table-core/src/features/row-aggregation/aggregationFns.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { constructAggregationFn } from './rowAggregationFeature.types'
import type { AggregationContext } from './rowAggregationFeature.types'

type RangeValue = Date | number

Expand All @@ -23,19 +22,9 @@ function compareRangeValues(left: RangeValue, right: RangeValue) {
return leftValue - rightValue
}

function collectRangeValues(context: AggregationContext<any, any, unknown>) {
const values: Array<RangeValue> = []
let kind: 'date' | 'number' | undefined

for (let i = 0; i < context.rows.length; i++) {
const value = context.getValue(context.rows[i]!)
const valueKind = getRangeKind(value)
if (!valueKind) continue
kind ??= valueKind
if (valueKind === kind) values.push(value as RangeValue)
}

return values
/** Comparable representation of a range value; `Date`s compare by time. */
function toRangeNumber(value: RangeValue): number {
return value instanceof Date ? value.getTime() : value
}

/**
Expand All @@ -49,9 +38,10 @@ export const aggregationFn_sum = constructAggregationFn<
number
>({
aggregate: (context) => {
const rows = context.rows
let sum = 0
for (let i = 0; i < context.rows.length; i++) {
const value = context.getValue(context.rows[i]!)
for (let i = 0; i < rows.length; i++) {
const value = context.getValue(rows[i]!)
sum += typeof value === 'number' ? value : 0
}
return sum
Expand All @@ -77,10 +67,23 @@ export const aggregationFn_min = constructAggregationFn<
RangeValue | undefined
>({
aggregate: (context) => {
const values = collectRangeValues(context)
let result = values[0]
for (let i = 1; i < values.length; i++) {
if (compareRangeValues(values[i]!, result!) < 0) result = values[i]
const rows = context.rows
let kind: 'date' | 'number' | undefined
let result: RangeValue | undefined
let resultNumber = 0
for (let i = 0; i < rows.length; i++) {
const value = context.getValue(rows[i]!)
const valueKind = getRangeKind(value)
if (!valueKind || (kind !== undefined && valueKind !== kind)) continue
const valueNumber = toRangeNumber(value as RangeValue)
if (kind === undefined) {
kind = valueKind
result = value as RangeValue
resultNumber = valueNumber
} else if (valueNumber - resultNumber < 0) {
result = value as RangeValue
resultNumber = valueNumber
}
}
return result
},
Expand Down Expand Up @@ -113,10 +116,23 @@ export const aggregationFn_max = constructAggregationFn<
RangeValue | undefined
>({
aggregate: (context) => {
const values = collectRangeValues(context)
let result = values[0]
for (let i = 1; i < values.length; i++) {
if (compareRangeValues(values[i]!, result!) > 0) result = values[i]
const rows = context.rows
let kind: 'date' | 'number' | undefined
let result: RangeValue | undefined
let resultNumber = 0
for (let i = 0; i < rows.length; i++) {
const value = context.getValue(rows[i]!)
const valueKind = getRangeKind(value)
if (!valueKind || (kind !== undefined && valueKind !== kind)) continue
const valueNumber = toRangeNumber(value as RangeValue)
if (kind === undefined) {
kind = valueKind
result = value as RangeValue
resultNumber = valueNumber
} else if (valueNumber - resultNumber > 0) {
result = value as RangeValue
resultNumber = valueNumber
}
}
return result
},
Expand Down Expand Up @@ -150,15 +166,33 @@ export const aggregationFn_extent = constructAggregationFn<
[RangeValue | undefined, RangeValue | undefined]
>({
aggregate: (context) => {
const values = collectRangeValues(context)
if (!values.length) return [undefined, undefined]
let min = values[0]!
let max = values[0]!
for (let i = 1; i < values.length; i++) {
const value = values[i]!
if (compareRangeValues(value, min) < 0) min = value
if (compareRangeValues(value, max) > 0) max = value
const rows = context.rows
let kind: 'date' | 'number' | undefined
let min: RangeValue | undefined
let max: RangeValue | undefined
let minNumber = 0
let maxNumber = 0
for (let i = 0; i < rows.length; i++) {
const value = context.getValue(rows[i]!)
const valueKind = getRangeKind(value)
if (!valueKind || (kind !== undefined && valueKind !== kind)) continue
const valueNumber = toRangeNumber(value as RangeValue)
if (kind === undefined) {
kind = valueKind
min = max = value as RangeValue
minNumber = maxNumber = valueNumber
} else {
if (valueNumber - minNumber < 0) {
min = value as RangeValue
minNumber = valueNumber
}
if (valueNumber - maxNumber > 0) {
max = value as RangeValue
maxNumber = valueNumber
}
}
}
if (kind === undefined) return [undefined, undefined]
return [min, max]
},
merge: ({ subRowResults }) => {
Expand Down Expand Up @@ -205,10 +239,11 @@ export const aggregationFn_mean = constructAggregationFn<
number | undefined
>({
aggregate: (context) => {
const rows = context.rows
let count = 0
let sum = 0
for (let i = 0; i < context.rows.length; i++) {
const value = context.getValue(context.rows[i]!)
for (let i = 0; i < rows.length; i++) {
const value = context.getValue(rows[i]!)
if (value == null) continue
const numberValue = typeof value === 'number' ? value : +value
if (!Number.isNaN(numberValue)) {
Expand All @@ -231,10 +266,11 @@ export const aggregationFn_median = constructAggregationFn<
number | undefined
>({
aggregate: (context) => {
if (!context.rows.length) return undefined
const values = new Array<number>(context.rows.length)
for (let i = 0; i < context.rows.length; i++) {
const value = context.getValue(context.rows[i]!)
const rows = context.rows
if (!rows.length) return undefined
const values = new Array<number>(rows.length)
for (let i = 0; i < rows.length; i++) {
const value = context.getValue(rows[i]!)
if (typeof value !== 'number') return undefined
values[i] = value
}
Expand All @@ -254,9 +290,10 @@ export const aggregationFn_unique = constructAggregationFn<
Array<unknown>
>({
aggregate: (context) => {
const rows = context.rows
const values = new Set<unknown>()
for (let i = 0; i < context.rows.length; i++) {
values.add(context.getValue(context.rows[i]!))
for (let i = 0; i < rows.length; i++) {
values.add(context.getValue(rows[i]!))
}
return Array.from(values)
},
Expand All @@ -270,9 +307,10 @@ export const aggregationFn_uniqueCount = constructAggregationFn<
number
>({
aggregate: (context) => {
const rows = context.rows
const values = new Set<unknown>()
for (let i = 0; i < context.rows.length; i++) {
values.add(context.getValue(context.rows[i]!))
for (let i = 0; i < rows.length; i++) {
values.add(context.getValue(rows[i]!))
}
return values.size
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { assignPrototypeAPIs, makeObjectMap } from '../../utils'
import { assignPrototypeAPIs } from '../../utils'
import {
cell_getIsAggregated,
column_getAggregationFns,
Expand Down Expand Up @@ -53,8 +53,4 @@ export const rowAggregationFeature: TableFeature = {
;(column as any)._aggregationValueCache = undefined
;(column as any)._resolvedAggregationFnsCache = undefined
},

initRowInstanceData: (row) => {
;(row as any)._aggregationValuesCache = makeObjectMap()
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@ export interface Cell_Aggregation {

/** Internal per-row cache used while grouped aggregates are evaluated. */
export interface Row_Aggregation {
/** Cached aggregate results keyed by column id. */
_aggregationValuesCache: Record<string, unknown>
/** Cached aggregate results keyed by column id; created lazily on grouped rows. */
_aggregationValuesCache?: Record<string, unknown>
}

/** Values passed to a column-level aggregation-value provider. */
Expand Down
Loading
Loading