Skip to content
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

Add extensive information about the driver in README #81

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

Gor027
Copy link
Contributor

@Gor027 Gor027 commented Sep 21, 2022

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@Gor027 Gor027 requested a review from avelanarius September 21, 2022 14:29
README.md Outdated
# ScyllaDB Cpp-Rust Driver
___
Wrapper around ScyllaDB's rust-driver compatible with Datastax cpp-driver.
As opposed to C/C++ driver, it works with the Cassandra Query Language v4 (CQL4).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by Cassandra Query Language v4? It looks like C/C++ driver works with CASS_PROTOCOL_VERSION_V4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was mentioned in the C++ driver description:

This driver works exclusively with the Cassandra Query Language v3 (CQL3) and Cassandra's native protocol.

Perhaps, it is outdated information.

README.md Outdated Show resolved Hide resolved
@avelanarius
Copy link

Please also add a section about limitations. Additionally, we should clean up the GitHub Issues that were opened - closing the ones that are now implemented.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the limitations section mentiond by @avelanarius is important - please write it before merging.

@Gor027
Copy link
Contributor Author

Gor027 commented Oct 10, 2022

Sure, I am working on it. I have updated the issues marking functions that are implemented and functions that are still in progress. I will base the limitations section on those issues unless you have another opinion.

@Gor027
Copy link
Contributor Author

Gor027 commented Oct 11, 2022

V2:

Added a separate section about the limitations of some features.
More information will be added to this section. For now, it covers the main limitations that the current implementation of the driver's features has.

@Lorak-mmk
Copy link
Collaborator

I'd like for the limitations section to include a note that the section itself may be incomplete - unless you are sure that the list is fairly complete?

One thing I'm not sure about:
In bindings.rs, the function that checks types is unimplemented:

pub fn is_compatible_type(_data_type: &CassDataType, _value: &Option<CqlValue>) -> bool {
// TODO: cppdriver actually checks types.
true
}

This I think is used for all binding - and you only noted that we are not checking types for cass_collection_append_* - is that correct?

@Gor027
Copy link
Contributor Author

Gor027 commented Oct 13, 2022

I'd like for the limitations section to include a note that the section itself may be incomplete - unless you are sure that the list is fairly complete?

Sure, I will add a note that the list is incomplete. For now, I have included the main functions which are not implemented or are implemented with some limitations. Functions that are relatively easy to implement (that do not require some implementation in the Rust driver, or maybe even in ScyllaDB) are just noted as unimplemented.

This I think is used for all binding - and you only noted that we are not checking types for cass_collection_append_* - is that correct?

What do you mean by all binding? The compatibility of types is being checked while appending or setting values in collections, tuples and UDTs. For binding a value to a prepared statement, the type is not being checked and in case of binding with incompatible type, the execution of the prepared statement should not be successful.

@Lorak-mmk
Copy link
Collaborator

Are you sure that prepared statements are not checked? I did a quick experiment: https://gist.github.com/Lorak-mmk/dbce7e0484b69000f8523df4ad963531

And for me it prints:

===== Using optimized driver!!! =====
bind 1 id: 0
bind 1 somestring: 0
execute 1: 0
bind 2 id: 0
bind 2 somestring: 16777229
execute 2: 0
DONE!

Which would mean that they are checked. After reading abstract_data.hh it also seems that the types should be checked for prepared statement.

@Gor027
Copy link
Contributor Author

Gor027 commented Oct 19, 2022

Which would mean that they are checked. After reading abstract_data.hh it also seems that the types should be checked for prepared statement.

Yeah, I guess you are right. The UserTypeValue and Statement classes extend from AbstractData class, so setting value by index or name should also check the type compatibility. Will fix the description.

Edit: The Rust driver, too, does not validate the types passed to queries here, however, we can add the validation of types independently from the Rust driver.

@Lorak-mmk
Copy link
Collaborator

There is already a placeholder function for that, it's just not implemented yet - always returns true.
See bindings.rs:53-56

@Lorak-mmk Lorak-mmk merged commit c8bf08b into scylladb:master Nov 10, 2022
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.

3 participants