Conversation
|
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. WalkthroughAdds a native-backed positional lookup API: Rust Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Builder as JSONBuilder
participant Binding as Native Binding (N-API)
participant Rust as get_in (Rust)
Client->>Builder: getIn(paths)
Builder->>Builder: getBufferIn(paths)
Builder->>Binding: native getIn(data, paths)
Binding->>Rust: invoke get_in(data, paths)
Rust->>Rust: validate paths, locate value, compute start/end
Rust-->>Binding: Some(GetInResult { start, end }) or None
Binding-->>Builder: GetInResult | null
alt offsets returned
Builder->>Builder: slice Buffer[start:end]
Builder->>Builder: JSON.parse(slice) -> value
Builder-->>Client: value
else not found
Builder-->>Client: undefined
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)src/json.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (9)
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 |
Summary of ChangesHello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces getIn and getBufferIn methods to the JSONBuilder, allowing for efficient retrieval of nested values from a JSON buffer by leveraging native Rust code to avoid parsing the entire document. The changes include the core implementation in Rust and JavaScript, along with corresponding type definitions, tests, and benchmarks. The implementation is solid, but I've identified a significant API inconsistency regarding the handling of empty path arrays. The JavaScript wrapper methods throw an error for empty paths, while the directly-exported native functions have a different behavior, which can even lead to panics in some existing functions. My review includes a recommendation to move this validation into the native Rust code for consistency and safety.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
benchmark/get_property_value.ts (1)
19-40: Consider benchmark methodology for more accurate comparison.The benchmark compares two different operations:
JSONParse: Parses the entire document and accesses the propertySonicJSONParse: Creates a JSONBuilder and retrieves only the specific propertyTwo recommendations:
Document the comparison intent: If the goal is to benchmark end-to-end property access (including parsing), the current approach is fine. However, if you want to isolate the property retrieval performance, consider moving the
JSONBuilderconstruction outside the timed loop.Reduce noise from builder construction: Line 26 creates a new
JSONBuilderinstance on each iteration, which includes construction overhead in the timing.If you want to measure only retrieval performance, consider this refactor:
function SonicJSONParse(data: Buffer) { const builder = new JSONBuilder(data) return builder.getIn<string>(['name']) } const parse = method === 'SonicJSONParse' ? SonicJSONParse : JSONParse +// Create builder once for SonicJSONParse to isolate retrieval performance +const builder = method === 'SonicJSONParse' ? new JSONBuilder(largeData) : null + const name = parse(largeData) console.log(`${method} get name of ${filename}: %o`, name) for (let i = 0; i < 5; i++) { const startTime = performance.now() - parse(largeData) + if (method === 'SonicJSONParse' && builder) { + builder.getIn<string>(['name']) + } else { + parse(largeData) + } const endTime = performance.now()Alternatively, if the intent is to benchmark the full operation including construction, consider adding a comment clarifying this design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
__test__/builder-get-in.spec.ts(1 hunks)benchmark/get_property_value.ts(1 hunks)index.d.ts(1 hunks)index.js(1 hunks)js/builder.ts(2 hunks)package.json(1 hunks)src/json.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
__test__/builder-get-in.spec.ts (1)
js/builder.ts (1)
JSONBuilder(20-134)
index.d.ts (1)
js/builder.ts (1)
getIn(107-114)
src/json.rs (2)
index.d.ts (1)
GetInResult(117-120)src/package.rs (1)
new(19-23)
benchmark/get_property_value.ts (2)
js/builder.ts (1)
JSONBuilder(20-134)benchmark/gc.ts (1)
getGCStats(51-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
index.js (1)
560-560: LGTM! Clean addition of the new native binding export.The changes correctly expose the
getInfunction from the native binding, following the established pattern for other exports likehasIn.Also applies to: 565-565
package.json (1)
59-61: LGTM! Benchmark script organization improved.The refactoring to separate benchmark scripts for different scenarios (
add_versionandget_property_value) improves maintainability and allows targeted benchmarking.index.d.ts (1)
115-120: LGTM! Type declarations are well-structured.The
GetInResultinterface andgetInfunction signature are correctly defined and align with the Rust implementation. The placement next tohasInmaintains good API organization.__test__/builder-get-in.spec.ts (1)
1-47: LGTM! Excellent test coverage.The test suite thoroughly validates the new
getInandgetBufferInfunctionality:
- Multiple data types (string, number, object, null, boolean, array)
- Nested paths
- Non-existent path handling
- Empty path validation
- Buffer representation accuracy
js/builder.ts (1)
5-5: LGTM! Clean implementation with proper delegation.The new methods are well-implemented:
getInappropriately delegates togetBufferInfor code reuse- Empty path validation is consistent with other methods (
setIn,deleteIn,hasIn)- Error handling follows established patterns
- The
@ts-expect-erroron line 112 is acceptable since the package targets Node.js >= 22 whereJSON.parseacceptsUint8ArrayAlso applies to: 107-125
src/json.rs (1)
241-259: LGTM! Solid Rust implementation following established patterns.The
get_infunction andGetInResultstruct are well-implemented:
- Mirrors the design of
has_in(lines 231-239) but returns positional metadata- Proper error handling: returns
Ok(None)for not found, propagates other errors- Uses the existing
get_value_positionhelper for consistency- Type definitions align with the TypeScript declarations in
index.d.ts
📦 napi-rs Build Size Comparison
Total: 849.82 KB → 853.82 KB (+4.00 KB, +.47%) Compared using x86_64-unknown-linux-gnu target |
There was a problem hiding this comment.
Pull request overview
This PR adds support for getIn and getBufferIn methods to the JSONBuilder class, allowing users to efficiently retrieve values from JSON data without parsing the entire document. The Rust backend provides a new get_in function that returns the byte positions of requested values, which the TypeScript methods use to extract and optionally parse the data.
Key changes:
- Added Rust
get_infunction that returns start/end byte positions of JSON values at specified paths - Added
getIn<T>()method to JSONBuilder that retrieves and parses values - Added
getBufferIn()method that returns the raw buffer slice without parsing - Added benchmark suite to compare performance with standard JSON.parse
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/json.rs | Implements get_in function and GetInResult struct in Rust for efficient position detection |
| js/builder.ts | Adds getIn and getBufferIn methods to JSONBuilder class |
| index.js | Exports the new getIn function from native bindings |
| index.d.ts | Adds TypeScript type definitions for getIn and GetInResult |
| test/builder-get-in.spec.ts | Adds comprehensive test suite for the new methods |
| benchmark/get_property_value.ts | Adds performance benchmark comparing JSONBuilder.getIn with JSON.parse |
| package.json | Updates benchmark scripts to include the new get_property_value benchmark |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
⚡ Benchmark ResultsClick to expand benchmark outputBenchmark ran on x86_64-unknown-linux-gnu target |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
__test__/builder-get-in.spec.ts (1)
49-55: Consider using valid JSON and adding more error test cases.The test partially addresses the past review comment about error scenarios, but could be improved:
The test uses malformed JSON (
'{"name": "John",'), which obscures whether the error is from invalid JSON parsing or from accessing a property of a non-object type. Consider using valid JSON like'{"name": "John"}'for clarity.Additional test cases are still missing for accessing properties of other non-object types in valid JSON:
['age', 'country']- accessing property of a number['bar', 'something']- accessing property of a boolean['foo', 'something']- accessing property of nullThese tests exist in
__test__/builder-has-in.spec.tsand would improve consistency across the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__test__/builder-get-in.spec.ts(1 hunks)benchmark/bench_memory.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
benchmark/bench_memory.ts (1)
benchmark/memory_usage.ts (1)
runMemoryBenchmarks(87-114)
__test__/builder-get-in.spec.ts (1)
js/builder.ts (1)
JSONBuilder(20-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Benchmark
- GitHub Check: stable - aarch64-pc-windows-msvc - node@24
- GitHub Check: stable - x86_64-pc-windows-msvc - node@24
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
benchmark/bench_memory.ts (1)
38-51: LGTM! Consistent with existing benchmark patterns.The new memory benchmarks for
get_property_valuefollow the exact same structure as the existinggetDescriptionandaddVersionbenchmarks, maintaining consistency in the test suite.__test__/builder-get-in.spec.ts (1)
5-47: Well-structured test coverage for the happy paths.The test cases comprehensively cover:
- Various data types (string, number, object, null, boolean, array)
- Non-existent property handling (undefined returns)
- Empty paths validation
- Both
getInandgetBufferInmethods with correct assertions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.