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

docs: adding context to timeuuid using v1 feature #980

Merged
merged 3 commits into from
May 13, 2024

Conversation

DanielHe4rt
Copy link
Contributor

@DanielHe4rt DanielHe4rt commented Apr 16, 2024

Motivation

I decided to give more context for newcomers that want to try Timeuuid in the Rust Driver. Since I didn't had that much experience in the beginning and struggled for a while until understand how to use it.

Also I still didn't understand when and how I should generate the the node_id on uuid::new_v1([]) parameters and IMHO it would be a great addition to the docs.

Fixes

  • Add context to uuid::v1 usage
  • (Optional) Add context to new_v1() node_id numbers

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.

Copy link

github-actions bot commented Apr 16, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 599a2be

@DanielHe4rt
Copy link
Contributor Author

Hmm apparently I can't ignore a snippet using the ,ignore flag and I also can't use the v1 flag in the extern crate...

Any ideas on how to make this work properly with extern crate or ignore?

image

@Lorak-mmk Lorak-mmk self-requested a review April 16, 2024 12:11
@wprzytula wprzytula requested review from wprzytula and muzarski April 23, 2024 09:23
@wprzytula
Copy link
Collaborator

I also can't use the v1 flag in the extern crate...

examples/Cargo.toml is the one used to configure deps for the docs. You can call cargo add uuid --dev -F v1 and the feature will be accessible from the docs.

@wprzytula
Copy link
Collaborator

Unfortunately, the error message from CI tells me nothing. As it's Sphinx task-related, it's @dgarcia360 who should have a clue about that.

@Lorak-mmk
Copy link
Collaborator

Unfortunately, the error message from CI tells me nothing. As it's Sphinx task-related, it's @dgarcia360 who should have a clue about that.

The problem is rust,ignore. We can't use that - our docs need to build both with mdbook and with Scylla's Sphinx setup, so code blocks can only use languages that are recognized by both.

Apart from incompatibility with Sphinx, I don't see why ignore is used - does this sample not compile for some reason? If so, the solution is to make it compile, not to use ignore

@wprzytula
Copy link
Collaborator

The problem is rust,ignore. We can't use that - our docs need to build both with mdbook and with Scylla's Sphinx setup, so code blocks can only use languages that are recognized by both.

Ah, right. Then adding v1 to uuid's feature list in examples/Cargo.toml and removing this ignore should fix the problem.

@wprzytula wprzytula added the area/documentation Improvements or additions to documentation label Apr 23, 2024
docs/source/data-types/timeuuid.md Outdated Show resolved Hide resolved
docs/source/data-types/timeuuid.md Outdated Show resolved Hide resolved
docs/source/data-types/timeuuid.md Outdated Show resolved Hide resolved
docs/source/data-types/timeuuid.md Show resolved Hide resolved
@DanielHe4rt
Copy link
Contributor Author

@annastuchlik Can you drop a review on it? Just in case I missed something :p

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.

One minor typo, apart from this looks good to me

docs/source/data-types/timeuuid.md Outdated Show resolved Hide resolved
@DanielHe4rt
Copy link
Contributor Author

Ready to merge!

docs/source/data-types/timeuuid.md Outdated Show resolved Hide resolved
docs/source/data-types/timeuuid.md Outdated Show resolved Hide resolved
Comment on lines 64 to 68
for row in rows.into_typed::<(CqlTimeuuid,)>() {
let (timeuuid_value,): (CqlTimeuuid,) = row?;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use raw rows field on QueryResult; prefer rows_typed::<(CqlTimeuuid,)>() method:

Suggested change
if let Some(rows) = session.query("SELECT a FROM keyspace.table", &[]).await?.rows {
for row in rows.into_typed::<(CqlTimeuuid,)>() {
let (timeuuid_value,): (CqlTimeuuid,) = row?;
}
}
let mut iter = session.query("SELECT a FROM keyspace.table", &[]).await?.rows_typed::<(CqlTimeuuid,)>();
while let Some((timeuuid_value,)) = iter.next().transpose()? {
// do something with timeuuid_value
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update, but can you explain in details why I shouldn't use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Ready to merge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

rows make you interact with an untyped, materialised vec of columns. At the present state of the driver, the main drawback is reduced type safety; as you call into_typed anyway, this negative aspect is mitigated.
Another reason for avoiding using rows ATM, which is relevant here, is the ongoing deserialization refactor. We ditch the materialized vec of CqlValue at all to reduce unnecessary allocations. The new deserialization framework is lazy, which means that the values are going to be deserialized on-the-fly straight to the end type, based on the type provided in the iterator (rows_typed::<T>).
With the new framework having landed in the driver, rows will be removed at all (although their capabilities can be emulated for the users that really need them).

See #955

@wprzytula wprzytula added the waiting-on-author Waiting for a response from issue/PR author label May 7, 2024
@wprzytula wprzytula removed the waiting-on-author Waiting for a response from issue/PR author label May 13, 2024
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution to the docs, @DanielHe4rt !

@wprzytula wprzytula merged commit 43044bd into scylladb:main May 13, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants