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.

Sign up for GitHub

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

Jump to bottom

Support CQL Vector type #1022

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Support CQL Vector type #1022

wants to merge 6 commits into from

Conversation

pkolaczk
Copy link

@pkolaczk pkolaczk commented Jun 26, 2024 •
edited
Loading

Fixes #1014

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@pkolaczk pkolaczk mentioned this pull request Jun 26, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 26, 2024 •
edited
Loading

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 9a19bbd

See the following report for details:

cargo semver-checks output 
./scripts/semver-checks.sh --baseline-rev bbb28746d96720764e5e476ef7ce11ba3a00a36d
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev bbb28746d96720764e5e476ef7ce11ba3a00a36d
     Cloning bbb28746d96720764e5e476ef7ce11ba3a00a36d
     Parsing scylla v0.13.0 (current)
      Parsed [  24.448s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  21.021s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.078s] 79 checks: 78 pass, 1 fail, 0 warn, 0 skip

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/enum_variant_added.ron

Failed in:
  variant CqlType:Vector in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/topology.rs:239

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  45.595s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [  10.448s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [  10.156s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.073s] 79 checks: 78 pass, 1 fail, 0 warn, 0 skip

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/enum_variant_added.ron

Failed in:
  variant ColumnType:Vector in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/frame/response/result.rs:75
  variant ParseError:InvalidCustomType in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/frame/frame_errors.rs:51
  variant CqlValue:Vector in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/frame/response/result.rs:101

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  20.722s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jun 26, 2024
@rukai
Copy link
Contributor

rukai commented Jun 26, 2024

Hi @pkolaczk I'm very happy for you to take on the final part of this work.
Since what I have in #1020 is quite small and includes tests, I'll keep responding to feedback there and hopefully merge it soon. After that it would be great to see your work land as a complete solution.

I also took some ideas from your PreCqlType implementation, thanks!

@rukai
Copy link
Contributor

rukai commented Jul 15, 2024

Heads up that while working with vector types I've discovered that even the official java driver has issues with them, it only supports vectors of int, long, float and double.

I've demonstrated the issues here: https://github.com/rukai/cassandra_vector_bug/blob/main/src/main/java/com/mycompany/app/App.java

@pkolaczk pkolaczk force-pushed the vector branch 2 times, most recently from a7d851e to dc4bc3d Compare August 13, 2024 09:06
@wprzytula wprzytula added the waiting-on-author Waiting for a response from issue/PR author label Aug 13, 2024
@pkolaczk pkolaczk force-pushed the vector branch 2 times, most recently from f65310c to f3eb389 Compare August 15, 2024 04:23
@pkolaczk pkolaczk force-pushed the vector branch 3 times, most recently from 8e89a68 to d18f001 Compare August 15, 2024 06:51
@pkolaczk pkolaczk mentioned this pull request Aug 16, 2024
Open
@pkolaczk
Copy link
Author

Heads up that while working with vector types I've discovered that even the official java driver has issues with them, it only supports vectors of int, long, float and double.

Internally only vectors of 32-bit floats are supported. This is what the index uses. There is no point in supporting other types in the driver at the moment.

@pkolaczk
Copy link
Author

Can you give me a few hints on how do I add tests to this work so I can get this moving forward?
Also, is the other Vector branch going to be merged soon (that one has tests)? I'm fine just adding serde on top of that other branch if that one is close to being ready, rather than repeat the work on the tests here.

hoxxep
hoxxep reviewed Sep 6, 2024
View reviewed changes
scylla-cql/src/frame/response/result.rs
@@ -112,6 +117,52 @@ pub enum CqlValue {
Varint(CqlVarint),
}


#[derive(Clone, Debug, PartialEq)]
pub struct DropOptimizedVec<T> {
Copy link

Choose a reason for hiding this comment

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

@pkolaczk I'm curious, do you have a microbenchmark that suggests this does anything different to a regular Vec<T> on release mode?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoxxep hoxxep hoxxep left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes waiting-on-author Waiting for a response from issue/PR author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CQL Vector type
4 participants
@pkolaczk @rukai @hoxxep @wprzytula