fix(ui): add bigint display support - #394
Conversation
|
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:
📝 WalkthroughWalkthroughBigInt support is added to statistic types and numeric formatters. DAG, query-plan, and pivot-table paths now format or aggregate bigint values, with tests covering formatter behavior, pivot aggregation, edge coloring, and edge widths. ChangesBigInt formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
ui/packages/@quent/utils/src/formatters.ts-121-126 (1)
121-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid coercing large BigInts through
Number.formatWithPrefixandinferFieldFormatterround values aboveNumber.MAX_SAFE_INTEGER, so large U64/I64 labels can drift. Add a bigint-safe fallback for unscaled output and cover >safe-integer cases informatters.test.ts.🤖 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/utils/src/formatters.ts around lines 121 - 126, Update formatWithPrefix and inferFieldFormatter in ui/packages/@quent/utils/src/formatters.ts at lines 121-126 and 291-293 to preserve bigint values without coercing them through Number, using a bigint-safe path for unscaled output while retaining existing formatting for safe values. Add regression coverage in ui/packages/@quent/utils/src/formatters.test.ts at lines 259-277 and 451-455 for values exceeding Number.MAX_SAFE_INTEGER, including large U64/I64 labels, and verify exact output without rounding.
🧹 Nitpick comments (1)
ui/packages/@quent/utils/src/formatters.test.ts (1)
259-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a bigint regression past
Number.MAX_SAFE_INTEGER. The current cases only use exactly representable values, so they won’t catch thebigint -> numbercoercion informatWithPrefix/inferFieldFormatter. Add one positive and one negative input above the safe-integer boundary and assert the expected display.🤖 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/utils/src/formatters.test.ts around lines 259 - 277, Extend the formatWithPrefix bigint tests with one positive and one negative value greater than Number.MAX_SAFE_INTEGER, asserting exact expected displays that preserve bigint precision. Place the regression cases in the existing bigint test coverage and ensure they exercise the formatWithPrefix/inferFieldFormatter conversion path.
🤖 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/utils/src/formatters.test.ts:
- Around line 255-257: Rename the colocated test file from formatters.test.ts to
formatters.test.tsx, preserving its existing Vitest test content and behavior.
---
Other comments:
In `@ui/packages/`@quent/utils/src/formatters.ts:
- Around line 121-126: Update formatWithPrefix and inferFieldFormatter in
ui/packages/@quent/utils/src/formatters.ts at lines 121-126 and 291-293 to
preserve bigint values without coercing them through Number, using a bigint-safe
path for unscaled output while retaining existing formatting for safe values.
Add regression coverage in ui/packages/@quent/utils/src/formatters.test.ts at
lines 259-277 and 451-455 for values exceeding Number.MAX_SAFE_INTEGER,
including large U64/I64 labels, and verify exact output without rounding.
---
Nitpick comments:
In `@ui/packages/`@quent/utils/src/formatters.test.ts:
- Around line 259-277: Extend the formatWithPrefix bigint tests with one
positive and one negative value greater than Number.MAX_SAFE_INTEGER, asserting
exact expected displays that preserve bigint precision. Place the regression
cases in the existing bigint test coverage and ensure they exercise the
formatWithPrefix/inferFieldFormatter conversion path.
🪄 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: ff47ed72-187c-484d-82bd-3588f5a8aec2
📒 Files selected for processing (5)
ui/packages/@quent/components/src/dag/DAGNodeInfoPanel.tsxui/packages/@quent/components/src/query-plan/QueryPlanNode.tsxui/packages/@quent/utils/src/dagTypes.tsui/packages/@quent/utils/src/formatters.test.tsui/packages/@quent/utils/src/formatters.ts
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/utils.ts:
- Around line 52-53: Update isNumericValue to compare typeof v against the
"bigint" type string, and retain the type predicate narrowing StatValue to
number | bigint so both numeric variants pass the guard.
🪄 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: 0062d330-9048-4c8f-bff3-223cae38310f
📒 Files selected for processing (1)
ui/packages/@quent/components/src/pivot-table/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/buildPivotedRows.test.ts:
- Line 159: Rename buildPivotedRows.test.ts to buildPivotedRows.test.tsx and
dagFieldProcessing.test.ts to dagFieldProcessing.test.tsx. Apply the same
dagFieldProcessing.test.tsx rename for both referenced tests, including the
width regression test; no test logic changes are required.
- Around line 160-176: Update isNumericValue in the aggregation path to detect
bigint values using the valid typeof string comparison, and update the
bucket.nums push logic in buildPivotedRows to coerce bigint row values to Number
before numeric reduction. Preserve existing handling for number values and
ensure bigint inputs continue through sum, mean, min, and max aggregation
without mixing numeric types.
In
`@ui/packages/`@quent/components/src/services/query-plan/dagFieldProcessing.test.ts:
- Around line 160-176: Update the numeric guards used by computeEdgeColoring and
computeEdgeWidthConfig in dagFieldProcessing.ts to accept values whose type is
either number or bigint. Preserve the existing numeric processing by coercing
accepted bigint values to number before calculating continuous coloring or
edge-width configuration.
🪄 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: b9dfd39c-4202-4cb7-b84d-5cdd5b92fafb
📒 Files selected for processing (2)
ui/packages/@quent/components/src/pivot-table/buildPivotedRows.test.tsui/packages/@quent/components/src/services/query-plan/dagFieldProcessing.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.test.ts:
- Around line 164-169: Rename the test file containing the formatStatValue
Vitest cases from utils.test.ts to utils.test.tsx, preserving all existing test
content and coverage.
- Around line 179-182: Fix the bigint handling introduced by the isNumericValue
test: correct the bigint guard in isNumericValue, then update its callers such
as getSortValue and aggregation paths to convert or otherwise widen bigint
values before number-only operations, including gradientBg and range
calculations. Preserve numeric behavior for existing number inputs and ensure
accepted bigint values cannot reach incompatible math unchanged.
In `@ui/packages/`@quent/utils/src/formatters.test.ts:
- Line 20: Fix the isNumericValue import in formatters.test.ts by locating the
module that defines and exports the predicate, then move the related checks
there or update the import to that module. Ensure the test no longer imports
isNumericValue from formatters.ts, which does not export it.
🪄 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: 290053fa-6376-41e1-834b-01c7fbf97f5c
📒 Files selected for processing (2)
ui/packages/@quent/components/src/pivot-table/utils.test.tsui/packages/@quent/utils/src/formatters.test.ts
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/utils.ts:
- Line 176: Update getSortValue and the aggregation bucket’s row.value handling
to preserve bigint values through sorting and aggregation instead of coercing
them with Number. Add bigint-aware comparisons and sum/mean/min/max
calculations, while retaining existing behavior for numeric non-bigint values
and returning the appropriate StatValue type.
🪄 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: 1f16c92c-a5fd-490f-b0ee-61d7061ccdf8
📒 Files selected for processing (6)
ui/packages/@quent/components/src/dag/DAGNodeInfoPanel.tsxui/packages/@quent/components/src/pivot-table/buildPivotedRows.test.tsui/packages/@quent/components/src/pivot-table/utils.test.tsui/packages/@quent/components/src/pivot-table/utils.tsui/packages/@quent/components/src/query-plan/QueryPlanNode.tsxui/packages/@quent/components/src/services/query-plan/dagFieldProcessing.test.ts
💤 Files with no reviewable changes (2)
- ui/packages/@quent/components/src/pivot-table/utils.test.ts
- ui/packages/@quent/components/src/pivot-table/buildPivotedRows.test.ts
| const v = row.values.get(stat); | ||
| if (v === undefined) return null; | ||
| return isNumericValue(v) ? v : null; | ||
| return isNumericValue(v) ? Number(v) : null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline ui/packages/@quent/components/src/pivot-table/utils.ts --view expanded || true
echo
echo "== relevant lines =="
sed -n '140,310p' ui/packages/@quent/components/src/pivot-table/utils.ts
echo
echo "== search for isNumericValue and related callers =="
rg -n "isNumericValue|parseJsonWithBigInt|Number\\(v\\)|pivot-table" ui/packages/@quent/components/src -SRepository: rapidsai/quent
Length of output: 10680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== utils.ts around numeric formatting and aggregation =="
sed -n '1,140p' ui/packages/@quent/components/src/pivot-table/utils.ts
sed -n '240,290p' ui/packages/@quent/components/src/pivot-table/utils.ts
echo
echo "== pivot-table types =="
sed -n '1,220p' ui/packages/@quent/components/src/pivot-table/types.ts
echo
echo "== search for StatValue and isNumericValue definitions =="
rg -n "type StatValue|interface StatValue|export .*isNumericValue|function isNumericValue|const isNumericValue" ui/packages/@quent -SRepository: rapidsai/quent
Length of output: 11009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== isNumericValue / formatter definitions in `@quent/utils` =="
rg -n "export .*isNumericValue|function isNumericValue|const isNumericValue|inferFieldFormatter|format.*bigint|StatValue" ui/packages/@quent/utils/src -S
echo
echo "== candidate files =="
fd -a "dagTypes.ts|.*utils.*" ui/packages/@quent/utils/src
echo
echo "== relevant source snippets =="
sed -n '1,140p' ui/packages/@quent/utils/src/dagTypes.ts
sed -n '1,220p' ui/packages/@quent/utils/src/index.tsRepository: rapidsai/quent
Length of output: 8520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getSortValue usages =="
rg -n "getSortValue\\(" ui/packages/@quent/components/src -S
echo
echo "== sort logic around pivoted rows =="
rg -n "sortInfo|compare|localeCompare|getSortValue|aggMode" ui/packages/@quent/components/src/pivot-table -S
echo
echo "== relevant chunks from pivot-table components =="
sed -n '1,260p' ui/packages/@quent/components/src/pivot-table/GroupedDataTable.tsx
sed -n '1,260p' ui/packages/@quent/components/src/pivot-table/PivotedStatTable.tsxRepository: rapidsai/quent
Length of output: 26159
Keep pivot-table stats as bigint through sort and aggregation
StatValue already includes bigint, but both Number(v) in getSortValue and Number(row.value) in the aggregation bucket silently narrow large u64 values. That can mis-order rows and produce wrong sums/means/min/max once values exceed Number.MAX_SAFE_INTEGER; use bigint-aware handling instead.
🤖 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 176, Update
getSortValue and the aggregation bucket’s row.value handling to preserve bigint
values through sorting and aggregation instead of coercing them with Number. Add
bigint-aware comparisons and sum/mean/min/max calculations, while retaining
existing behavior for numeric non-bigint values and returning the appropriate
StatValue type.
Source: Path instructions
There was a problem hiding this comment.
This too, but if you want I can go back and do the pivot table stuff
There was a problem hiding this comment.
@johallar, yes please—updating the pivot-table path in this PR would be best, since it is part of the BigInt-support objective. In particular, preserve exact bigint values for sorting and aggregate operations so large u64 statistics remain correctly ordered and calculated, and add regression coverage for values beyond Number.MAX_SAFE_INTEGER.
There was a problem hiding this comment.
@johallar Ok, just pushed up a fix for this
There was a problem hiding this comment.
Nice, I see the agg fixes, but the line that this comment is on will still affect sorting won't it?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Ah right, should've mentioned it - I was thinking we could address this in a follow-up PR, as that would require cascading changes to gradientBg and the column range comparisons in PivotedStatsTable.tsx. Here would be the three areas where we are currently accepting precision loss, which would require that refactor:
getSortValueline 176 - sort order for non-aggregated rows with large bigintsgetSortValueagg path - agg.sum, agg.min, agg.max come fromPivotedRowAggwhich storesnumber | null, so even though we now compute them more accurately as bigints, they're converted to number before being stored and returned heregradientBgcolor scaling - uses the number sort value to position a value on a color gradient, so large bigints that lost precision ingetSortValueget slightly wrong colors
I'm not quite sure how large those changes would be, so I thought keeping it separate might make sense.
There was a problem hiding this comment.
👍 that works! Let's just track in an issue so we don't lose track of it
cmatzenbach
added
bug
cmatzenbach
changed the title
|
One change I want to make for this issue is to use the typescript binding for |
|
|
||
| const abs = value < 0 ? -value : value; | ||
| const sign = value < 0 ? '-' : ''; | ||
| if (num === 0) return symbol ? `0 ${symbol}` : '0'; |
There was a problem hiding this comment.
bigint 0 does not triple equal number 0 😒 0 === 0n -> false
loose equality or another check for num === 0n would fix here
There was a problem hiding this comment.
Probably outdated now based on your following comment, but yes, BigInt is coerced to ensure all values are number types here
| decimals: number = 1 | ||
| ): string { | ||
| if (value === 0) return symbol ? `0 ${symbol}` : '0'; | ||
| const num = typeof value === 'bigint' ? Number(value) : value; |
There was a problem hiding this comment.
Converting BigInt to Number is going to be a problem if above the max safe integer value
There was a problem hiding this comment.
That was acknowledged and addressed that in the table in the PR description above; the precision loss doesn't affect the output here. The rounding done by formatWithPrefix already throws away far more precision than the BigInt -> number conversion would, so the precision loss is invisible in the final output.
The place where the precision loss would matter is in formatNumber, which is handled differently. Instead we are passing BigInt to Intl.NumberFormat to ensure no conversion happens.
There was a problem hiding this comment.
Ah I see, I think the case where this matters most is prefixSystem === "None", which would call toFixed on the coerced Number. If there was precision loss on the BigInt conversion to Number that would still be shown here.
Since we have the BigInt already, I think we want to do the scaling for units using the full precision if possible. Here's a failing unit test for the "None" case:
it('preserves unprefixed bigint precision above Number.MAX_SAFE_INTEGER', () => {
expect(formatWithPrefix(9007199254740993n, 'Hz', 'None', 0)).toBe(
'9007199254740993 Hz'
);
});
There's one other case where losing the precision can cause it to roll into another prefix. Less concerned about this, but an edge case that could be fixed:
it('normalizes a rounded mantissa into the next prefix', () => {
expect(formatWithPrefix(999999999999999n, 'Hz', 'Si', 1)).toBe('1.0 PHz');
});
There was a problem hiding this comment.
Ah that's a great catch. Added a new path for BigInt values when prefixSystem is disabled so they are no longer coerced.
Also included one other small change AI had found earlier related to the Terra prefix - when the divided mantissa rounds up to 1000, we now step up to the next prefix instead of displaying 1000.0 THz.
Agreed - went ahead and derived that from the BE-generated |
| if (fieldName.endsWith('_mbs')) return v => `${v.toFixed(1)} MB/s`; | ||
| if ( | ||
| export function inferFieldFormatter(fieldName: string): (value: number | bigint) => string { | ||
| let format: (value: number) => string; |
There was a problem hiding this comment.
This should be number | bigint shouldn't it?
There was a problem hiding this comment.
Assuming you're talking about the format variable, no - the BigInt conversion is handled at line 368 in the returned wrapper: value => format(typeof value === 'bigint' ? Number(value) : value)
| if (fieldName.endsWith('_ns')) format = v => formatDuration(v / 1e6); | ||
| else if (isBytesStat(fieldName)) format = v => formatBytes(v, 2); | ||
| else if (isCountStat(fieldName)) format = v => formatSiCount(v, 2); | ||
| else if (fieldName.endsWith('_mbs')) format = v => `${v.toFixed(1)} MB/s`; |
There was a problem hiding this comment.
I think this + the ratio/fraction block below is problematic for BigInts, they don't have a toFixed method
There was a problem hiding this comment.
These lambdas take number - the conversion from BigInt to number happens at line 368 before format is ever called. By the time any of these run, v is already a number, so .toFixed() works fine.
| format = v => `${(v * 100).toFixed(1)}%`; | ||
| else format = v => formatNumberWithMaxFractionDigits(v, 4); | ||
|
|
||
| return value => format(typeof value === 'bigint' ? Number(value) : value); |
There was a problem hiding this comment.
This coercion is saving the formatters above from type errors, but prevents the bigint from ever reaching formatWithPrefix
There was a problem hiding this comment.
So I had looked at this before, but the formatters in this function - durations, ratios, percentages, counts - all do float arithmetic that doesn't make sense on a BigInt anyway. The only formatter where preserving BigInt precision matters is formatBytes, which calls formatWithPrefix. However, byte counts from the API are unlikely to exceed Number.MAX_SAFE_INTEGER in practice (that's what, 9 petabytes?), and even if they did, formatBytes applies IEC prefix scaling which rounds to 1 decimal place, so any precision loss in the Number() conversion would be invisible in the output.
There was a problem hiding this comment.
This makes sense, but we are making a big assumption based on the field name as to what its probable values are at this point. The fact is that any of these DynamicValue type statistics can be 64 bit numbers, and it's going to depend on the engine developer who is implementing the instrumentation what these values are.
It would feel better to really cover our bases and handle all the (numeric) types instead of leaning on the Number coercion IMO.
eg. fields with _ns that are nanosecond epoch timestamps are going to exceed MAX_SAFE_INTEGER and display incorrect values when coerced to Number, but also if any of the other name based bucketing is not correct we could end up in that same situation
There was a problem hiding this comment.
Or actually...maybe if serde serializes these values with the type we can just have two code paths?
There was a problem hiding this comment.
Ok, here's another idea that addresses some values that could be valid bigints:
export function inferFieldFormatter(fieldName: string): (value: number | bigint) => string {
return (value: number | bigint): string => {
const num = typeof value === 'bigint' ? Number(value) : value;
if (fieldName.endsWith('_ns')) return formatDuration(num / 1e6);
if (isBytesStat(fieldName)) return formatBytes(value, 2);
if (isCountStat(fieldName)) return formatSiCount(num, 2);
if (fieldName.endsWith('_mbs')) return `${num.toFixed(1)} MB/s`;
if (
fieldName.endsWith('_ratio') ||
fieldName.endsWith('_fraction') ||
fieldName.endsWith('_fpr') ||
fieldName.endsWith('_selectivity') ||
fieldName.endsWith('_rate')
)
return `${(num * 100).toFixed(1)}%`;
return typeof value === 'bigint' ? formatNumber(value) : formatNumberWithMaxFractionDigits(num, 4);
};
The inferFieldFormatter function now handles bigints with two explicit paths rather than blindly coercing everything to Number upfront:
- formatBytes receives the original bigint, which
formatWithPrefixalready handles natively - The default case uses
formatNumber(value)for bigints, which formats viaIntl.NumberFormat - All other branches (_ns, counts, ratios, _mbs) still convert to
Numbersince their math is float-only
Now, for the paths do still coerce to Number, here's my thought - there's really a limit to how far you can go here. For the ratio/percentage branch, if a value arrives as a bigint and you multiply it by 100 and call toFixed, the output is almost certainly wrong semantically too - a bigint named _ratio is a misuse of the field naming convention, and no amount of bigint-safe math will make the display meaningful. If the engine developer names a field _ratio and emits a u64, the display is wrong regardless - bigint safety doesn't fix that semantic mismatch. Let me know what you think.
|
/merge |
2 participants
Description
Previously the DAG stats panel and tooltips did not support
BigInt, so they would be coerced into strings and not go through the formatting functions. This addsBigIntsupport to the formatting functions (and opens up pathways soBigIntvalues can access them) so that they will be treated like other numeric values.Other changes:
isNumericValueinto@quent/utilsto use as a general utility function, avoiding repeated logicBigInt(coerced into numbers) rather than dropping themBigIntfields keep continuous coloring rather than falling back to categoricalBigIntpathsNote: Since we are coercing
BigIntstonumbersin several areas, I had claude do an analysis on the safety of each of these coercions in terms of what it affects in the application. Here is a chart showing each place and its risk factor:Related Issues
Closes: #351
Testing
A set of unit tests were added to ensure all new
BigIntpaths are verified.If you have a ndjson file with
BigIntstats (I do not), that would be the best test scenarioScreenshots