Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add support for advanced decoding CQL BigInt #137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 52 commits into
base: second-iteration
Choose a base branch
Loading
from

Conversation

adespawn
Copy link
Collaborator

No description provided.

Stapox35 and others added 29 commits March 20, 2025 10:32
Create a row of a result as a Row class instead of plain js object,
to keep parity with datastax driver.

Extend interface of Row constructor, so it can be created
either from array of strings or array of objects
with property name representing column name
(the second options was present in Datastax driver).
Enables tests for executing prepared queries. Test related to error throwing check only if error is thrown, instead of checking if error is of expected type.
Minor changes to code style:
- Update hashset to class
 - Remove deprecated uses of Buffer()
- Update string formatting to new format
Move tests for LocalDate to new file.
Add tests for errors in setters.
Add implementation of Rust object with constracor and Cql getter/setter.
Add some custom functions for date operations.
Add getters and setters.
Improved existing functions to use the rust object.
Implement to_format() with Display trait.
This function in JS called toString().
Between rust and JS and value into QueryParameterWrapper.
Enable `with date and time types` integration test.
Move code related to request logic into separate folder.
Currently this objects keeps only some of the parameters exposed in the API.
Remaining parameters will need to be added at a later time.

(All skipped options were due to complexity of the expected type)
Add conversion function that takes query QueryOptionsWrapper
and apply it to the prepared statement.

This takes use of only some of the QueryOptionsWrapper fields.
Converts js queryOptions into queryOptionsWrapper
Move UUID regex, so it's accessible from other parts of the code
Make use of the existing type guessing done by datastax driver
Update the queries, so they use type guessing.
Fixes the driver so that no longer all queries are prepared.
Converts all allowed types of parametr hints into Complex Type
used by the driver. Currently this task is split between rust and JS,
but it can be later fully moved to the rust part of the code.s

Make use of napi object

In function convert hint, instead of checking the types manually,
move this job to NAPI-RS, by making use of napi objects.

As info field can be either just a element, or array of elements,
on the JS side I convert all non null objects into array of objects,
and than convert back to single element on rust side.
Why this test was previously working?
Before this PR, all queries were treated as prepared queries.
When this was changes, to not prepared(*), the error no longer relies
on the js code, ensuring correct amount of parameters (see cql utils: parseParams)
and now relies on the error returned by the database.

With the lack of correct error mapping between rust driver and js driver,
the error currently thrown is no longer Response error, but other error type:

GenericFailure
Error: Error#Serializing values failed: SerializationError:
Failed to type check query arguments
alloc::vec::Vec<core::option::Option<scylla_cql::frame::response::result::CqlValue>>:
wrong column count: the statement operates on 2 columns, but the given rust type provides 1
Add ability to execute unprepared batch queries,
and enables some integration tests for this feature.
adespawn and others added 13 commits March 20, 2025 10:33
Datastax driver allows to provide partial hints, for complex types:
like map or list. This commit adds support for this feature,
and enables appropriate integration tests.
Make the names more idiomatic
This test should not have been enabled in "Update and enable some integration tests"
Deprecated modules are added to the except list of Typescript tests.
The modules are available in JS but not in TS to fail early when used.
Typescript does not allow case as a variable name.
In pedantic mode any nodeJS warning will be treated as an error
PiotrJunior and others added 10 commits March 20, 2025 10:52
Integration tests appear to have issues in certain conditions,
when run on github CI, which depends on the order in which they are run,
which tests are enabled (there is no single failing test).
I was unable to reproduce the issue on any other machine than Github workers.

The issue appears as one of the following problems:
- capacity overflow
- memory allocation failure

After some investigation, while I wasn't able to pinpoint a specific issue,
I determined that the issue arises from concurrent JS code interacting
with the NAPI-RS layer.

This commit attempts to stop the problems, by removing the concurrency.
This feature is controlled by NO_CONCURRENCY env variable.
The concurrency is removed, only when this variable is set.
This severely limits the speed at which executeConcurrent is performed,
but appears to fully resolve the issue.

With this in mind, any concurrency should be implemented on Rust side,
or the root issue should be determined before removing the concurrency restrictions.
Ensure map keys and elements are not null.
To have parity with the old driver, following types can be provided
as a string in expected format instead of providing object of given class type.

Timestamp
Intet
UUID
TimeUUID
LocalDate
LocalTime
As we decided, we no longer want to accept numbers as strings.
This commits removes (currently disabled) tests for those encodings.
Allows to decode CQL BigInt into either BigInt type or Long type,
according to provided configuration option.

This still requires the configuration option to be correctly passed
from the option provided by the client into the rust part of the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.