fix(getRange): cannot support multiple pk#8
Conversation
|
Warning Rate limit exceeded@BlackHole1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughThis change introduces support for infinite boundary constants (INF_MIN and INF_MAX) for range query operations. Two new constants are added to the public API as empty objects. The GetRangeData interface is updated to accept arrays of primary key components for inclusiveStartPrimaryKey and exclusiveEndPrimaryKey instead of single values. Internal logic in fixPlainBufferCellType is modified to recognize and handle these infinity constants when inferring variant types. Documentation is updated to demonstrate the new usage pattern with array-based boundaries combined with infinity constants. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the GetRange operation to support composite primary keys with infinity bounds. The key changes include:
- Introduced
INF_MINandINF_MAXconstants for specifying infinite bounds in range queries - Updated
GetRangeto accept arrays of primary key cells instead of single cells - Enhanced type inference to properly handle infinity values
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/const.ts | Adds INF_MIN and INF_MAX constants as empty objects to represent infinity bounds |
| src/index.ts | Exports the new INF_MIN and INF_MAX constants |
| src/plainbuffer.ts | Imports infinity constants (unused in diff but needed for future logic) |
| src/utils.ts | Updates fixPlainBufferCellType to detect infinity constants and assign correct VariantType |
| src/operator/get-range.ts | Changes primary key parameters from single cell to arrays, imports VariantType |
| README.md | Updates documentation with example showing composite primary keys using infinity bounds |
Comments suppressed due to low confidence (2)
src/operator/get-range.ts:8
- Unused import VariantType.
import { decodePlainBuffer, encodePlainBuffer, VariantType } from "../plainbuffer";
src/plainbuffer.ts:1
- Unused imports INF_MAX, INF_MIN.
import { INF_MAX, INF_MIN } from "./const";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| inclusiveStartPrimaryKey: createPrimaryKey("id", "100"), | ||
| exclusiveEndPrimaryKey: createPrimaryKey("id", "200"), | ||
| inclusiveStartPrimaryKey: [ | ||
| createPrimaryKey("id", "100") |
There was a problem hiding this comment.
Missing comma after createPrimaryKey('id', '100') in the array literal.
| createPrimaryKey("id", "100") | |
| createPrimaryKey("id", "100"), |
src/plainbuffer.ts
Outdated
| import { INF_MAX, INF_MIN } from "./const"; | ||
|
|
There was a problem hiding this comment.
The imported constants INF_MIN and INF_MAX are not used in this file. The actual logic to handle these constants exists in src/utils.ts. Consider removing this unused import or add a comment explaining why it's needed for future use.
| import { INF_MAX, INF_MIN } from "./const"; |
src/operator/get-range.ts
Outdated
| import { OTS_API_NAME } from "../const"; | ||
| import { builder } from "../pb/builder"; | ||
| import { decodePlainBuffer, encodePlainBuffer } from "../plainbuffer"; | ||
| import { decodePlainBuffer, encodePlainBuffer, VariantType } from "../plainbuffer"; |
There was a problem hiding this comment.
The imported VariantType is not used in this file. Consider removing it from the import statement.
| import { decodePlainBuffer, encodePlainBuffer, VariantType } from "../plainbuffer"; | |
| import { decodePlainBuffer, encodePlainBuffer } from "../plainbuffer"; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/const.ts (1)
26-27: Add documentation and freeze sentinel constants.These sentinel constants should be immutable and documented. Consider applying
Object.freeze()to prevent accidental mutation and adding JSDoc comments to explain their purpose in multi-key range queries.Apply this diff:
+/** + * Sentinel value representing negative infinity for range query boundaries. + * Use this with GetRange to specify an unbounded lower limit on a primary key component. + */ -export const INF_MIN = {}; +export const INF_MIN = Object.freeze({}); + +/** + * Sentinel value representing positive infinity for range query boundaries. + * Use this with GetRange to specify an unbounded upper limit on a primary key component. + */ -export const INF_MAX = {}; +export const INF_MAX = Object.freeze({});src/operator/get-range.ts (1)
21-22: Breaking change correctly implements multi-key range support.The change from single
PlainBufferCelltoPlainBufferCell[]properly enables composite primary key ranges. Consider adding validation to ensure arrays are non-empty, and JSDoc comments to clarify the multi-key capability.Optional: Add validation in the
buildermethod to prevent empty arrays:public static async builder(options: GetRangeData) { if (!options.inclusiveStartPrimaryKey.length || !options.exclusiveEndPrimaryKey.length) { throw new Error("Primary key arrays cannot be empty"); } // ... rest of the method }And consider adding JSDoc to the interface:
export interface GetRangeData { tableName: string; direction: typeof Direction[keyof typeof Direction]; columnsToGet?: string[]; timeRange?: TimeRange; maxVersions?: number; limit?: number; + /** Array of primary key components defining the inclusive start boundary. Use INF_MIN for unbounded lower limits. */ inclusiveStartPrimaryKey: PlainBufferCell[]; + /** Array of primary key components defining the exclusive end boundary. Use INF_MAX for unbounded upper limits. */ exclusiveEndPrimaryKey: PlainBufferCell[]; filter?: Filter; startColumn?: string; endColumn?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/const.ts(1 hunks)src/index.ts(1 hunks)src/operator/get-range.ts(3 hunks)src/plainbuffer.ts(1 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/operator/get-range.ts (2)
src/plainbuffer.ts (2)
PlainBufferCell(653-653)encodePlainBuffer(651-651)src/utils.ts (1)
fixPlainBufferCellType(5-19)
src/utils.ts (2)
src/plainbuffer.ts (2)
PlainBufferCell(653-653)VariantType(656-656)src/const.ts (2)
INF_MIN(26-26)INF_MAX(27-27)
src/const.ts (1)
src/index.ts (2)
INF_MIN(7-7)INF_MAX(6-6)
🪛 GitHub Actions: PR
src/operator/get-range.ts
[error] 8-8: ESLint: 'VariantType' is defined but never used. (unused-imports/no-unused-imports)
src/plainbuffer.ts
[error] 1-1: ESLint: 'INF_MAX' is defined but never used. (unused-imports/no-unused-imports)
[error] 1-1: ESLint: 'INF_MIN' is defined but never used. (unused-imports/no-unused-imports)
🔇 Additional comments (3)
src/index.ts (1)
6-7: LGTM! Public API appropriately extended.The addition of
INF_MAXandINF_MINto the public exports correctly supports the new multi-key range boundary feature for GetRange operations.src/utils.ts (1)
1-19: LGTM! Sentinel value handling correctly implemented.The addition of
INF_MINandINF_MAXchecks using reference equality (===) is the correct approach for object-based sentinels. The logic properly handles these special values before falling back to type inference, maintaining backward compatibility.src/operator/get-range.ts (1)
43-47: LGTM! Array mapping correctly processes multi-key boundaries.The
.map(fixPlainBufferCellType)approach correctly processes each primary key component, ensuring proper type inference and INF sentinel handling for composite keys.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const INF_MIN = {}; | ||
| export const INF_MAX = {}; |
There was a problem hiding this comment.
The INF_MIN and INF_MAX constants are defined as empty objects {}, but the PlainBufferCell interface only allows value to be bigint | number | boolean | string | Uint8Array. This creates a type mismatch that TypeScript should flag. Additionally, using object identity (===) for sentinel values can be fragile. Consider using unique symbols instead:
export const INF_MIN = Symbol('INF_MIN');
export const INF_MAX = Symbol('INF_MAX');This would provide type safety and clear semantic intent while maintaining reference equality checks.
| export const INF_MIN = {}; | |
| export const INF_MAX = {}; | |
| export const INF_MIN = Symbol('INF_MIN'); | |
| export const INF_MAX = Symbol('INF_MAX'); |
|
|
||
| export const INF_MIN = {}; |
There was a problem hiding this comment.
Missing documentation for the exported INF_MIN and INF_MAX constants. These are special sentinel values used in range queries and should be documented to explain their purpose (e.g., "Sentinel value representing negative infinity for range query bounds" and "Sentinel value representing positive infinity for range query bounds").
| export const INF_MIN = {}; | |
| /** | |
| * Sentinel value representing negative infinity for range query bounds. | |
| * Use this as the lower bound in range queries to indicate no minimum. | |
| */ | |
| export const INF_MIN = {}; | |
| /** | |
| * Sentinel value representing positive infinity for range query bounds. | |
| * Use this as the upper bound in range queries to indicate no maximum. | |
| */ |
| import type { PlainBufferCell } from "./plainbuffer"; | ||
| import { INF_MAX, INF_MIN } from "./const"; |
There was a problem hiding this comment.
Import order should follow a consistent pattern. Consider grouping imports by source: type imports first, then value imports from the same module. The current order has PlainBufferCell (type) imported from "./plainbuffer", then imports from "./const", then more imports from "./plainbuffer". Suggest reordering to:
import { INF_MAX, INF_MIN } from "./const";
import type { PlainBufferCell } from "./plainbuffer";
import { inferVariantType, VariantType } from "./plainbuffer";| import type { PlainBufferCell } from "./plainbuffer"; | |
| import { INF_MAX, INF_MIN } from "./const"; | |
| import { INF_MAX, INF_MIN } from "./const"; | |
| import type { PlainBufferCell } from "./plainbuffer"; |
src/operator/get-range.ts
Outdated
| import { OTS_API_NAME } from "../const"; | ||
| import { builder } from "../pb/builder"; | ||
| import { decodePlainBuffer, encodePlainBuffer } from "../plainbuffer"; | ||
| import { decodePlainBuffer, encodePlainBuffer, VariantType } from "../plainbuffer"; |
There was a problem hiding this comment.
Unused import VariantType.
| import { decodePlainBuffer, encodePlainBuffer, VariantType } from "../plainbuffer"; | |
| import { decodePlainBuffer, encodePlainBuffer } from "../plainbuffer"; |
503dfb1 to
fc524cf
Compare
Signed-off-by: Kevin Cui <bh@bugs.cc>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Kevin Cui <bh@bugs.cc>
fc524cf to
c58297f
Compare
No description provided.