fix: skip non-numeric values in median aggregation - #6523
Merged
KevinVandy merged 2 commits intoAug 7, 2026
Conversation
aggregationFn_median bailed out and returned undefined for an entire group as soon as it hit a single non-numeric (e.g. null) value, unlike sum/min/max/mean which all skip non-numeric values and aggregate over whatever numeric values remain. Bring median in line with the rest of the built-in aggregation functions: filter out non-numeric values and compute the median from what's left, only returning undefined when no numeric values remain. Fixes TanStack#5008
Contributor
📝 WalkthroughWalkthroughThe median aggregation now ignores nullish and non-numeric values. It returns ChangesMedian aggregation behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
packages/table-core/tests/unit/fns/aggregationFns.test.ts (1)
72-78: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the string-filtering assertions distinguish skipping from coercion.
[3, '2', 1, 2]and[null, undefined, 4, '5', 6]produce the same median whether the string is ignored or coerced. Add a case such as[1, '100', 3]with an expected median of2.Proposed test
+ expect(aggregationFn_median.aggregate(context([1, '100', 3]))).toBe(2)Also applies to: 89-91
🤖 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/tests/unit/fns/aggregationFns.test.ts` around lines 72 - 78, Update the median assertions in the test “preserves mean coercion and median numeric-only behavior” to include a case such as [1, '100', 3] expecting 2, clearly verifying that numeric strings are skipped rather than coerced. Apply the same distinction to the additional median assertion around the referenced range while preserving the existing mean coercion coverage.
🤖 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 `@packages/table-core/src/features/row-aggregation/aggregationFns.ts`:
- Around line 273-275: Update the median aggregation input filter in the
relevant aggregation function to push values only when they are numbers and not
NaN, so all-NaN inputs return undefined and mixed inputs ignore NaN. Add
regression cases covering [NaN] and [NaN, 1].
---
Nitpick comments:
In `@packages/table-core/tests/unit/fns/aggregationFns.test.ts`:
- Around line 72-78: Update the median assertions in the test “preserves mean
coercion and median numeric-only behavior” to include a case such as [1, '100',
3] expecting 2, clearly verifying that numeric strings are skipped rather than
coerced. Apply the same distinction to the additional median assertion around
the referenced range while preserving the existing mean coercion coverage.
🪄 Autofix
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 Plus
Run ID: 293bdc5d-c1a4-4ee9-a072-40018dd624b5
📒 Files selected for processing (2)
packages/table-core/src/features/row-aggregation/aggregationFns.tspackages/table-core/tests/unit/fns/aggregationFns.test.ts
Comment on lines
273
to
275
| const value = context.getValue(rows[i]!) | ||
| if (typeof value !== 'number') return undefined | ||
| values[i] = value | ||
| if (typeof value === 'number') values.push(value) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant symbols:"
git ls-files | rg 'packages/table-core/src/features/row-aggregation/aggregationFns\.ts|aggregationFns\.ts' || true
echo
echo "Relevant lines:"
if [ -f packages/table-core/src/features/row-aggregation/aggregationFns.ts ]; then
nl -ba packages/table-core/src/features/row-aggregation/aggregationFns.ts | sed -n '230,305p'
fi
echo
echo "Search for median/mean implementations:"
rg -n "aggregationFn_median|aggregationFn_mean|Number\.isNaN|typeof value === 'number'" packages/table-core/src/features/row-aggregation/aggregationFns.ts packages/table-core/src || true
echo
echo "Behavioral probe for median-like filter:"
node - <<'JS'
function currentMedianValues(values) {
const out = []
for (const value of values) {
if (typeof value === 'number') out.push(value)
}
return out
}
for (const input of [[NaN], [NaN, 1], [1, NaN], [NaN, 2, 3]]) {
console.log(JSON.stringify(input), '=>', JSON.stringify(currentMedianValues(input)))
}
console.log('typeof NaN === "number":', typeof NaN === 'number')
console.log('Number.isNaN(NaN):', Number.isNaN(NaN))
console.log('median-like value:', Number.isNaN(input => {
const filtered = input.filter(x => typeof x === 'number' && !Number.isNaN(x))
filtered.sort((a,b) => a-b)
const midIdx = Math.floor(filtered.length / 2)
return filtered.length % 2 === 0 && filtered.length > 0
? (filtered[midIdx - 1] + filtered[midIdx]) / 2
: filtered[midIdx]
})([NaN, 1]) ? 'not computed' : undefined)
JSRepository: TanStack/table
Length of output: 475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant lines:"
awk '{printf "%6d\t%s\n", NR, $0}' packages/table-core/src/features/row-aggregation/aggregationFns.ts | sed -n '230,305p'
echo
echo "Search for median/mean implementations:"
rg -n "aggregationFn_median|aggregationFn_mean|Number\.isNaN|typeof value === 'number'|" packages/table-core/src/features/row-aggregation/aggregationFns.ts packages/table-core/src || true
echo
echo "Behavioral probe:"
node - <<'JS'
function currentValues(values) {
const out = []
for (const value of values) {
if (typeof value === 'number') out.push(value)
}
return out
}
for (const input of [[NaN], [NaN, 1], [1, NaN], [NaN, 2, 3]]) {
console.log(JSON.stringify(input), '=>', JSON.stringify(currentValues(input)))
}
console.log('typeof NaN === "number":', typeof NaN === 'number')
console.log('Number.isNaN(NaN):', Number.isNaN(NaN))
function medianLike(values) {
const filtered = currentValues(values)
const n = filtered.length
if (!n) return NaN
const sorted = [...filtered].sort((a,b) => a-b)
const mid = Math.floor(n / 2)
return n % 2 === 0 ? (sorted[mid - 1] + sorted[mid]) / 2 : sorted[mid]
}
console.log('current median([NaN]):', medianLike([NaN]))
console.log('current median([NaN, 1]):', medianLike([NaN, 1]))
JSRepository: TanStack/table
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "aggregatedFn section:"
awk '{printf "%6d\t%s\n", NR, $0}' packages/table-core/src/features/row-aggregation/aggregationFns.ts | sed -n '250,290p'
echo
echo "Number.isNaN occurrences:"
rg -n "Number\.isNaN|type === 'number'|values\.sort|median|mean" packages/table-core/src/features/row-aggregation/aggregationFns.ts
echo
echo "Behavioral probe:"
node - <<'JS'
function currentValues(values) {
const out = []
for (const value of values) {
if (typeof value === 'number') out.push(value)
}
return out
}
function sortedMedian(values) {
const filtered = currentValues(values)
const sorted = [...filtered].sort((a,b) => a-b)
const n = sorted.length
const mid = Math.floor(n / 2)
return n % 2 === 0 && n > 0 ? (sorted[mid - 1] + sorted[mid]) / 2 : sorted[mid]
}
for (const input of [[NaN], [NaN, 1], [1, NaN], [NaN, 2, 3]]) {
console.log(JSON.stringify(input), '=> values', JSON.stringify(currentValues(input)), 'median', sortedMedian(input))
}
console.log('typeof NaN === "number":', typeof NaN === 'number')
console.log('Number.isNaN(NaN):', Number.isNaN(NaN))
console.log('proposed median([NaN]):', null)
JSRepository: TanStack/table
Length of output: 2302
Exclude NaN from median inputs.
typeof value === 'number' includes NaN, so [NaN] returns NaN instead of undefined. Add !Number.isNaN(value) before pushing numeric values, and add regression cases for [NaN] and [NaN, 1].
🤖 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/row-aggregation/aggregationFns.ts` around
lines 273 - 275, Update the median aggregation input filter in the relevant
aggregation function to push values only when they are numbers and not NaN, so
all-NaN inputs return undefined and mixed inputs ignore NaN. Add regression
cases covering [NaN] and [NaN, 1].
Source: MCP tools
|
View your CI Pipeline Execution ↗ for commit 4169e7e
☁️ Nx Cloud last updated this comment at |
KevinVandy
approved these changes
Aug 7, 2026
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.
Changes
aggregationFn_medianbailed out and returnedundefinedfor the whole group the moment it hit a single non-numeric value (e.g.null). That's inconsistent with every other built-in aggregation function —sum,min,max, andmeanall skip non-numeric/nullish values and aggregate over whatever numeric values remain instead of discarding the group.This brings
medianin line with its siblings: it now filters out non-numeric values first, computes the median from what's left, and only returnsundefinedwhen no numeric values remain at all.Fixes #5008
Note on behavior change
This does change the previously-asserted result for groups that contain non-numeric values. The existing test had:
That assertion documented the buggy early-return behavior, not an intentional design choice — there's no rationale for it in the aggregation-overhaul PRs, and it directly contradicts how
sum/min/max/meanalready handle the same situation. I updated it to reflect the fixed behavior (median of the numeric values[3, 1, 2]->2) and added a dedicated regression test covering: anullinside a group, a mix ofnull/undefined/non-numeric-string, an even-length numeric result, and the all-non-numeric ->undefinedfallback case.Testing
pnpm --filter @tanstack/table-core test:lib— 62 files / 1272 tests passingpnpm --filter @tanstack/table-core test:eslint— cleanpnpm --filter @tanstack/table-core test:types— cleanSummary by CodeRabbit
Bug Fixes
Tests