Support groupBy over BigInt and Date
#1091
Conversation
|
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: +5 B (+0.01%) Total Size: 89.5 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.35 kB ℹ️ View Unchanged
|
kevin-dp
force-pushed
the
kevin/group-by-bigint-or-date
branch
from
860214a to
b590c96
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for groupBy operations over BigInt and Date column types, fixing issue #1085 where JSON.stringify would fail with "Do not know how to serialize a BigInt" errors.
Key changes:
- Introduced a
serializeValueutility function that handles BigInt and Date serialization using a custom JSON.stringify replacer - Updated groupBy operations to use
serializeValueinstead ofJSON.stringifyfor creating group keys - Added comprehensive test coverage for single and multi-column groupBy scenarios with BigInt and Date types
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db-ivm/src/utils.ts | Adds serializeValue function to handle BigInt and Date serialization for groupBy keys |
| packages/db-ivm/src/operators/groupBy.ts | Updates groupBy operator to use serializeValue for key string generation |
| packages/db/src/query/compiler/group-by.ts | Updates multi-column groupBy key generation to use serializeValue |
| packages/db-ivm/src/index.ts | Exports serializeValue utility function |
| packages/db/tests/query/group-by.test.ts | Adds four comprehensive tests covering BigInt and Date groupBy scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function serializeValue(value: unknown): string { | ||
| return JSON.stringify(value, (_, val) => { | ||
| if (typeof val === 'bigint') { | ||
| return val.toString() | ||
| } | ||
| if (val instanceof Date) { | ||
| return val.toISOString() | ||
| } | ||
| return val | ||
| }) | ||
| } |
There was a problem hiding this comment.
The new serializeValue utility function lacks unit tests in the db-ivm package. While integration tests exist in the db package, consider adding unit tests in packages/db-ivm/tests/utils.test.ts to verify edge cases like null values, undefined values, nested objects with BigInt/Date, arrays containing BigInt/Date, and mixed types. This would improve maintainability and ensure the function behaves correctly in isolation.
…ons (#1093) * Initial plan * Replace hardcoded BigInt values with Number.MAX_SAFE_INTEGER + 1 Co-authored-by: kevin-dp <[email protected]> * Extract MAX_SAFE_BIGINT constant to improve readability Co-authored-by: kevin-dp <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: kevin-dp <[email protected]>
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Code Review: PR #1091
Overall Assessment: Approve ✅
A clean, focused fix for a real-world issue. Like the foundation stones of a temple, proper serialization is essential for the structure above to function correctly.
Summary
The PR adds a serializeValue utility function that properly handles BigInt and Date types during JSON serialization, which JSON.stringify cannot do natively. This is then used in the groupBy operator for key serialization.
Strengths
-
Minimal, targeted change: Only touches the necessary files (utils.ts, groupBy.ts, group-by.ts)
-
Good test coverage: The tests cover:
- BigInt values beyond
MAX_SAFE_INTEGER(important edge case) - Date values
- Multi-column grouping with mixed types
- Grouping verification with duplicate keys
- BigInt values beyond
-
Clean implementation: The replacer function approach is idiomatic:
JSON.stringify(value, (_, val) => { if (typeof val === 'bigint') return val.toString() if (val instanceof Date) return val.toISOString() return val })
-
Exported for reuse:
serializeValueis exported from the package index, making it available for other consumers.
Minor Notes
-
Potential precision considerations: For Dates,
toISOString()returns millisecond precision. If you ever need microsecond/nanosecond precision, this would need revisiting. Not an issue for current use cases. -
Two serialization sites: The fix is applied in two places:
packages/db-ivm/src/operators/groupBy.ts:71- key extractionpackages/db/src/query/compiler/group-by.ts:257- final key construction
Both are necessary and correctly placed.
-
Consider adding NaN handling:
JSON.stringify(NaN)returnsnull, which could cause unexpected grouping. Not blocking, but worth considering for robustness.
Question
Are there other places in the codebase using JSON.stringify on user data that might also benefit from serializeValue? A quick search might reveal other potential issues.
Solid work! 🎯
Fixes #1085