feat(ui): pivot table bigint support - #480
Conversation
cmatzenbach
added
non-breaking
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesPivot table bigint support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@ui/packages/`@quent/components/src/pivot-table/PivotedStatTable.tsx:
- Around line 45-46: Update the mixed-type comparison branch in the comparator
containing the `typeof a === typeof b` check to compare the original numeric
operands directly with `<` instead of converting either value through
`Number()`. Preserve the existing -1/1 ordering behavior while retaining
precision for mixed number/bigint values.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8db16fd9-d164-4c6d-8f58-e9f388eb96c3
📒 Files selected for processing (4)
ui/packages/@quent/components/src/pivot-table/PivotedStatTable.tsxui/packages/@quent/components/src/pivot-table/buildPivotedRows.test.tsui/packages/@quent/components/src/pivot-table/types.tsui/packages/@quent/components/src/pivot-table/utils.ts
|
|
||
| const HIGHLIGHT_WASH = 'inset 0 0 0 999px hsl(var(--primary) / 0.07)'; | ||
|
|
||
| const numericSortingFn: SortingFn<PivotedRow> = (rowA, rowB, columnId) => { |
There was a problem hiding this comment.
Better to live in operator table utils
There was a problem hiding this comment.
Good point, moved it over
johallar
left a comment
johallar
left a comment
There was a problem hiding this comment.
One organization thing, otherwise 🙇 tyty
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/packages/@quent/components/src/pivot-table/utils.ts (1)
297-308: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not silently coerce mixed bigint buckets to
number.Exact aggregation is applied only when every numeric input is bigint. If a bucket contains both
numberandbigint, the surrounding path converts bigint values withNumber()before computing sum/min/max, which can produce incorrect displayed and sorted aggregates above2^53. Define an exact mixed-type policy instead of silently rounding these values.🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts around lines 297 - 308, The aggregation branch around onlyBigints must explicitly handle mixed number/bigint buckets instead of converting bigint values through Number() and risking precision loss. Preserve exact bigint arithmetic when all inputs are bigint, and define an explicit policy for mixed values—such as rejecting them or marking aggregates unsupported—so displayed and sorted sum/min/max results are never silently rounded above 2^53.
🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts:
- Around line 35-43: Update numericSortingFn’s type annotation to match its
comparator return values: either import TanStack’s SortingFn and export the
function as a typed const, or retain the function declaration with a number
return type. Ensure the chosen approach avoids referencing an unimported
SortingFn.
---
Outside diff comments:
In `@ui/packages/`@quent/components/src/pivot-table/utils.ts:
- Around line 297-308: The aggregation branch around onlyBigints must explicitly
handle mixed number/bigint buckets instead of converting bigint values through
Number() and risking precision loss. Preserve exact bigint arithmetic when all
inputs are bigint, and define an explicit policy for mixed values—such as
rejecting them or marking aggregates unsupported—so displayed and sorted
sum/min/max results are never silently rounded above 2^53.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: e021430e-11d4-4f42-90e4-51581d941a7f
📒 Files selected for processing (2)
ui/packages/@quent/components/src/pivot-table/PivotedStatTable.tsxui/packages/@quent/components/src/pivot-table/utils.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/packages/@quent/components/src/pivot-table/utils.ts (2)
285-287: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep bigint inputs out of the stdev path
Casting bigint values to
Numberloses precision aboveNumber.MAX_SAFE_INTEGER, so variance/stdev can become wrong or even collapse to0while sum/min/max stay exact. Keep the variance calculation in exact bigint arithmetic and convert only the final result if needed.🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts around lines 285 - 287, Update the variance/stdev preparation around allNums so bigint inputs are not converted to Number before calculation. Keep bigint-only variance arithmetic exact, while preserving the existing mixed-number behavior and converting only the final stdev result when required by the API.
38-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle equal mixed numeric values as equal. Mixed
number/bigintvalues like1nand1fall through to the fallback and return1in both directions, which breaks sort ordering. Compare with both<and>and return0when neither is true.🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts around lines 38 - 42, Update the mixed-type comparison fallback in the comparator around the visible a and b checks to evaluate both less-than and greater-than relationships, returning 0 when neither is true. Preserve the existing ordering for unequal numeric values and the earlier null and same-value handling.
♻️ Duplicate comments (1)
ui/packages/@quent/components/src/pivot-table/utils.ts (1)
35-35: 🎯 Functional Correctness | 🔴 CriticalThe previous comparator type error is still present.
numericSortingFnreturns-1,0, or1, but Line 35 declares the return type asSortingFn<PivotedRow>, which is itself a function type. Change the return type tonumber, or declare aconstcomparator typed asSortingFn<PivotedRow>.🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts at line 35, Update numericSortingFn so its declared return type is number, matching the -1/0/1 comparator result it returns; do not use SortingFn<PivotedRow> as the function’s return type.
🤖 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.
Outside diff comments:
In `@ui/packages/`@quent/components/src/pivot-table/utils.ts:
- Around line 285-287: Update the variance/stdev preparation around allNums so
bigint inputs are not converted to Number before calculation. Keep bigint-only
variance arithmetic exact, while preserving the existing mixed-number behavior
and converting only the final stdev result when required by the API.
- Around line 38-42: Update the mixed-type comparison fallback in the comparator
around the visible a and b checks to evaluate both less-than and greater-than
relationships, returning 0 when neither is true. Preserve the existing ordering
for unequal numeric values and the earlier null and same-value handling.
---
Duplicate comments:
In `@ui/packages/`@quent/components/src/pivot-table/utils.ts:
- Line 35: Update numericSortingFn so its declared return type is number,
matching the -1/0/1 comparator result it returns; do not use
SortingFn<PivotedRow> as the function’s return type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: b7458548-030b-4938-90f1-0640c8889990
📒 Files selected for processing (2)
ui/packages/@quent/components/src/pivot-table/PivotedStatTable.tsxui/packages/@quent/components/src/pivot-table/utils.ts
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/packages/@quent/components/src/pivot-table/utils.ts (2)
39-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn
0for numerically equal mixed values.For example,
a = 1andb = 1nbypassa === b, then the fallback returns1; the reverse comparison also returns1. This violates the comparator contract and can produce incorrect sorting. Compare both<and>and return0when neither is true.🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts around lines 39 - 43, Update the comparator’s mixed-type fallback after the null and same-type checks to evaluate both less-than and greater-than relationships, returning 0 when neither is true. Preserve the existing -1/1 ordering for strictly smaller or larger values, including numerically equal values such as number and bigint pairs.
69-81: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve precision when determining bigint gradient ranges.
Distinct large values such as
9007199254740992nand9007199254740993nboth convert to the sameNumber, causingmn === mxand disabling the gradient for a non-empty range. Keep bigint range comparisons exact and only normalize the interpolation ratio in a way that does not collapse supported values.🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts around lines 69 - 81, Update gradientBg so bigint min and max are compared exactly before calculating the gradient, avoiding Number conversion that collapses distinct large bigint values. Preserve the existing number behavior, and compute the interpolation ratio using a precision-safe normalization that supports the accepted bigint range before passing it to continuousColor.
🟠 Other critical/major comments (1)
ui/packages/@quent/components/src/pivot-table/utils.ts-7-7 (1)
7-7: 🎯 Functional Correctness | 🔴 CriticalUse
SortingFnas the function type, not the return type.
SortingFn<PivotedRow>describes the comparator function, but this declaration saysnumericSortingFnreturns a comparator while it returns numbers. This remains a compile-time error; define a typed const or annotate the function parameters and return type asnumber.Also applies to: 36-44
🤖 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 `@ui/packages/`@quent/components/src/pivot-table/utils.ts at line 7, Update numericSortingFn to use SortingFn<PivotedRow> as the comparator function type rather than its return type, or explicitly type its parameters and numeric return value. Ensure the declaration matches the function’s actual number-returning implementation and resolves the compile-time error.
🤖 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.
Outside diff comments:
In `@ui/packages/`@quent/components/src/pivot-table/utils.ts:
- Around line 39-43: Update the comparator’s mixed-type fallback after the null
and same-type checks to evaluate both less-than and greater-than relationships,
returning 0 when neither is true. Preserve the existing -1/1 ordering for
strictly smaller or larger values, including numerically equal values such as
number and bigint pairs.
- Around line 69-81: Update gradientBg so bigint min and max are compared
exactly before calculating the gradient, avoiding Number conversion that
collapses distinct large bigint values. Preserve the existing number behavior,
and compute the interpolation ratio using a precision-safe normalization that
supports the accepted bigint range before passing it to continuousColor.
---
Other critical/major comments:
In `@ui/packages/`@quent/components/src/pivot-table/utils.ts:
- Line 7: Update numericSortingFn to use SortingFn<PivotedRow> as the comparator
function type rather than its return type, or explicitly type its parameters and
numeric return value. Ensure the declaration matches the function’s actual
number-returning implementation and resolves the compile-time error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: a2e9fc87-a993-427d-891d-4e9d3e550727
📒 Files selected for processing (1)
ui/packages/@quent/components/src/pivot-table/utils.ts
|
/merge |
2 participants
Description
Modifies pivot table sorting function, gradient background generation, stats computation, and typescript definitions to support
BigIntwithout converting toNumber. The only places we are still coercingBigIntare where it is needed - for mean computation and gradient coloring, where precision loss is acceptable. In addition, updates unit tests to test forBigIntwithout coercing.Related Issues
Closes #475
Testing
ndjsonfile withBigIntstatistics, ensure the pivot table functions as normal