Varint #195
Varint #195
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces client options support for Varint conversions, allowing Varint values to be interpreted as either Integer or BigInt depending on the client-specified options. Key changes include:
- Addition of tests (both JavaScript and Rust) to validate Varint conversion behavior.
- Updates in encoding/decoding logic for Varint in both CQL utility and request/response wrapper code.
- Adjustments in the client and parameter parsing functions to pass through client options.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/query-parameter-tests.js | Added tests to verify Varint conversion, including BigInt support. |
| test/unit/cql-value-wrapper-tests.js | Extended tests for Varint handling through N-API functions. |
| src/tests/result_tests.rs | Introduced a Rust test for returning a Varint CqlValueWrapper. |
| src/tests/request_values_tests.rs | Added Varint mapping in value tests using both Integer and BigInt. |
| src/result.rs | Added a new method to retrieve Varint buffers. |
| src/requests/parameter_wrappers.rs | Implemented a new factory function for creating varint query parameters. |
| lib/types/results-wrapper.js | Updated getCqlObject to support Varint with client options. |
| lib/types/result-set.js | Modified calls to getRowsFromResultsWrapper to pass client options. |
| lib/types/cql-utils.js | Enhanced parameter encoding functions to forward options for Varint. |
| lib/new-utils.js | Added utilities for encoding and decoding Varint values with BigInt. |
| lib/client.js | Propagated client options in various query and batch request methods. |
lib/types/results-wrapper.js
Outdated
| @@ -58,12 +61,15 @@ function getCqlObject(field) { | ||
| return new Date(Number(field.getTimestamp())); | ||
| case rust.CqlType.List: | ||
| fields = field.getList(); | ||
| return fields.map((value) => getCqlObject(value)); | ||
| return fields.map((value) => getCqlObject(value), options); |
There was a problem hiding this comment.
Passing 'options' as the second argument to Array.map sets the 'this' binding but does not pass it into getCqlObject. Update the code to explicitly pass options by changing it to: return fields.map((value) => getCqlObject(value, options));
| return fields.map((value) => getCqlObject(value), options); | |
| return fields.map((value) => getCqlObject(value, options)); |
Copilot uses AI. Check for mistakes.
|
|
|
As previously discussed, please re-base onto #183 |
Before, we created QueryParameterWrapper with from_... functions, and then cloning this object when executing query. Now, with implementation of FromNapiValue trait, we pass parameters to queries directly, without any additional conversion steps. In order to correctly handle value types, each parameter is passed as a pair (type, value). Information about type is then used on the rust side to create correct CqlValue object.
Added new utils for decoding and encoding Varint from and to Integer and BigInt.
Adds support for convertin JS Varint type into Rust Varint type.
Added tests to verify Varint encoding.
Adds support for converting Varint responses to a JavaScript object.
Added tests to verify Varint decoding.
| if (typeof value === "string") { | ||
| // All numeric types are supported as strings for historical reasons | ||
| value = BigInt(value); |
There was a problem hiding this comment.
Haven't we decided to drop support for providing numerical values as strings? @adespawn
There was a problem hiding this comment.
That was for non big int integers.
Here, I believe it's a different case and we should consider it separately from the "normal" integers
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Varint types by enabling conversions between Integer and BigInt representations in various client and server modules. Key changes include:
- New test cases verifying Varint handling in query parameter and CQL value wrapper tests.
- Updates to the conversion logic in Rust and JavaScript to support Varint encoding/decoding.
- Enhancements in new utility functions to handle conversion for Varint values.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/query-parameter-tests.js | Adds Varint test cases using both Integer.fromString and BigInt. |
| test/unit/cql-value-wrapper-tests.js | Introduces a test for retrieving Varint type via napi. |
| src/tests/result_tests.rs | Adds a test for Varint conversion in CQL value wrappers. |
| src/tests/request_values_tests.rs | Updates value resolution to include Varint support. |
| src/result.rs | Implements conversion for CqlValue::Varint using add_type_to_napi_obj. |
| src/requests/parameter_wrappers.rs | Updates Varint conversion logic from napi Buffer to CqlVarint. |
| lib/types/results-wrapper.js | Handles Varint type in JavaScript results conversion. |
| lib/types/cql-utils.js | Modifies value wrapping to support Varint via encodeVarint. |
| lib/new-utils.js | Adds utility functions for encoding and decoding Varint values. |
| } | ||
| if (typeof value === "string") { | ||
| value = Integer.fromString(value); | ||
| } | ||
| let buf = null; | ||
| if (typeof value === "bigint") { | ||
| buf = encodeVarintFromBigInt(value); | ||
| } | ||
| if (value instanceof Buffer) { | ||
| buf = value; | ||
| } | ||
| if (value instanceof Integer) { |
There was a problem hiding this comment.
[nitpick] Consider refactoring the series of type checks in encodeVarint into an if-else chain to ensure that only one conversion branch is executed, which can improve readability and reduce potential conversion ambiguities.
| } | |
| if (typeof value === "string") { | |
| value = Integer.fromString(value); | |
| } | |
| let buf = null; | |
| if (typeof value === "bigint") { | |
| buf = encodeVarintFromBigInt(value); | |
| } | |
| if (value instanceof Buffer) { | |
| buf = value; | |
| } | |
| if (value instanceof Integer) { | |
| } else if (typeof value === "string") { | |
| value = Integer.fromString(value); | |
| } | |
| let buf = null; | |
| if (typeof value === "bigint") { | |
| buf = encodeVarintFromBigInt(value); | |
| } else if (value instanceof Buffer) { | |
| buf = value; | |
| } else if (value instanceof Integer) { |
Copilot uses AI. Check for mistakes.
| * @param {Buffer} bytes | ||
| * @returns {BigInt} | ||
| */ | ||
| function decodeVarintAsBigInt(bytes) { |
There was a problem hiding this comment.
[nitpick] It would be helpful to add a brief comment or JSDoc note about the expected type and format of the 'bytes' parameter in decodeVarintAsBigInt to aid future maintainers in understanding its assumptions.
Copilot uses AI. Check for mistakes.
1dfdd1c to
95900c8
Compare
| @@ -23,6 +24,22 @@ const errorTypeMap = { | ||
|
|
||
| const concatenationMark = "#"; | ||
|
|
||
| // BigInt constants | ||
| const isBigIntSupported = typeof BigInt !== "undefined"; |
There was a problem hiding this comment.
BigInt was introduced in Node10. (see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#browser_compatibility). I don't think we want to support version of node that are end-of-life for the last 3-4 years
| * Converts a value to a varint buffer. | ||
| * @param {Integer|Buffer|String|Number} value | ||
| * @returns {Buffer} | ||
| * @private |
There was a problem hiding this comment.
https://jsdoc.app/tags-package
I think this tag should be used here
| if (typeof value === "string") { | ||
| // All numeric types are supported as strings for historical reasons | ||
| value = BigInt(value); |
There was a problem hiding this comment.
That was for non big int integers.
Here, I believe it's a different case and we should consider it separately from the "normal" integers
| CqlValue::Varint(val) => add_type_to_napi_obj( | ||
| env, | ||
| Buffer::to_napi_value(env, Buffer::from(val.as_signed_bytes_be_slice())), | ||
| CqlType::Varint, | ||
| ), |
There was a problem hiding this comment.
Unless you can provide some benchmarks, that show performance differences, I believe it's better to convert bytes into BigInt on the Rust side of the code
| CqlType::Varint => CqlValue::Varint(CqlVarint::from_signed_bytes_be( | ||
| get_element!(Buffer).to_vec(), | ||
| )), |
There was a problem hiding this comment.
Compared to result, I'm open to discussion whether we should convert everything to Buffer or BigInt on the JS side, or should we use mixed approach (everything except buffer is provided as BigInt and Buffer is provided as buffer)
| /** | ||
| * Converts a BigInt to a varint buffer. | ||
| * @param {String|BigInt} value | ||
| * @returns {Buffer} | ||
| */ | ||
| function encodeVarintFromBigInt(value) { |
There was a problem hiding this comment.
I want to see a lot of unit tests for this function (unless you decide to switch the approach)
| /** | ||
| * Converts a byte array to a BigInt | ||
| * @param {Buffer} bytes | ||
| * @returns {BigInt} | ||
| */ | ||
| function decodeVarintAsBigInt(bytes) { |
There was a problem hiding this comment.
(Ditto) I want to see a lot of unit tests for this function (unless you decide to switch the approach)
Varint can be either Integer or BigInt depending on ClientOptions.
Added ClientOptions "pass-through" for
getWrappedandgetCqlObjectfunctions in order that to work.