value: mark CqlValue as non_exhaustive #1257

muzarski · 2025-02-24T17:07:43Z

Motivation

The motivation is quite obvious - we want to be able to extend CqlValue without introducing API breaking changes in the future.

Obstacles

When you simply try to add non_exhaustive to CqlValue, the compiler starts to complain. The reason is that CqlValue is defined in scylla-cql crate, and we match against its instance in CqlValueDisplayer (scylla crate). The compiler complains that _ arm is not covered - which makes sense.

We could obviously add the _ arm, but it's not ok. In the future, if someone adds new variant to CqlValue, the compliler won't complain about uncovered variant in CqlValueDisplayer. It will result in runtime error (or panic) when someone tries to display a CqlValue instance holding a new variant. So, TLDR, all commits but last are related to cleaning up, and moving pretty module from scylla to scylla-cql.

Things to discuss

chrono 0.4 - Very important!!!

In current version of PR, chrono 0.4 dependency is no longer optional (and hidden behind chrono-04 feature). It's needed unconditionally for some conversions used in std::fmt::Display implementation for CqlValue. I'm not sure how to address it. I don't see (or know of) any alternative solution to this...

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.~~

github-actions · 2025-02-24T17:13:17Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev c24db7759804e1563d21bd92d466d33fdb9aacce
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev c24db7759804e1563d21bd92d466d33fdb9aacce
     Cloning c24db7759804e1563d21bd92d466d33fdb9aacce
    Building scylla v0.15.0 (current)
       Built [  25.777s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.052s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.793s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.053s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.128s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  49.758s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.707s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.028s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.436s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.028s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.115s] 127 checks: 126 pass, 1 fail, 0 warn, 0 skip

--- failure enum_marked_non_exhaustive: enum marked #[non_exhaustive] ---

Description:
A public enum has been marked #[non_exhaustive]. Pattern-matching on it outside of its crate must now include a wildcard pattern like `_`, or it will fail to compile.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#attr-adding-non-exhaustive
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/enum_marked_non_exhaustive.ron

Failed in:
  enum CqlValue in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/value.rs:808

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

Lorak-mmk

After moving displayer to scylla-cql, is it not possible to remove chrono dependency from scylla crate?

scylla-cql/src/value.rs

Lorak-mmk · 2025-02-24T18:21:21Z

scylla-cql/src/value.rs

+                CqlValue::Date(CqlDate(d)) => {
+                    // This is basically a copy of the code used in `impl TryInto<NaiveDate> for CqlDate` impl
+                    // in scylla-cql. We can't call this impl because it is behind chrono feature in scylla-cql.
+
+                    // date_days is u32 then converted to i64 then we subtract 2^31;
+                    // Max value is 2^31, min value is -2^31. Both values can safely fit in chrono::Duration, this call won't panic
+                    let days_since_epoch =
+                        chrono_04::Duration::try_days(*d as i64 - (1 << 31)).unwrap();
+
+                    // TODO: chrono::NaiveDate does not handle the whole range
+                    // supported by the `date` datatype
+                    match chrono_04::NaiveDate::from_ymd_opt(1970, 1, 1)
+                        .unwrap()
+                        .checked_add_signed(days_since_epoch)
+                    {
+                        Some(d) => write!(f, "'{}'", d)?,
+                        None => f.write_str("<date out of representable range>")?,
+                    }
+                }
+                CqlValue::Duration(d) => write!(f, "{}mo{}d{}ns", d.months, d.days, d.nanoseconds)?,
+                CqlValue::Time(CqlTime(t)) => {
+                    write!(
+                        f,
+                        "'{:02}:{:02}:{:02}.{:09}'",
+                        t / 3_600_000_000_000,
+                        t / 60_000_000_000 % 60,
+                        t / 1_000_000_000 % 60,
+                        t % 1_000_000_000,
+                    )?;
+                }
+                CqlValue::Timestamp(CqlTimestamp(t)) => {
+                    match chrono_04::Utc.timestamp_millis_opt(*t) {
+                        chrono_04::LocalResult::Ambiguous(_, _) => unreachable!(), // not supposed to happen with timestamp_millis_opt
+                        chrono_04::LocalResult::Single(d) => {
+                            write!(f, "{}", d.format("'%Y-%m-%d %H:%M:%S%.3f%z'"))?
+                        }
+                        chrono_04::LocalResult::None => {
+                            f.write_str("<timestamp out of representable range>")?
+                        }
+                    }
+                }


One solution - admittedly not ideal - is to have separate display impls when the feature is enabled and not.
Something like this:

CqlValue::Date(CqlDate(d)) => { #[cfg(feature = "chrono-04")] { // This is basically a copy of the code used in `impl TryInto<NaiveDate> for CqlDate` impl // in scylla-cql. We can't call this impl because it is behind chrono feature in scylla-cql. // date_days is u32 then converted to i64 then we subtract 2^31; // Max value is 2^31, min value is -2^31. Both values can safely fit in chrono::Duration, this call won't panic let days_since_epoch = chrono_04::Duration::try_days(*d as i64 - (1 << 31)).unwrap(); // TODO: chrono::NaiveDate does not handle the whole range // supported by the `date` datatype match chrono_04::NaiveDate::from_ymd_opt(1970, 1, 1) .unwrap() .checked_add_signed(days_since_epoch) { Some(d) => write!(f, "'{}'", d)?, None => f.write_str("<date out of representable range>")?, } } #[cfg(not(feature = "chrono-04"))] { let days_since_epoch = *d as i64 - (1 << 31); write!(f, "{} days from epoch", days_since_epoch) } }

It allows to keep chrono optional, but has 2 downsides:

if we add more chrono versions, then we will need to put more implementations here and select based on cfg expressions which I don't like. It may not be a big problem, as chrono seems to bump major version very rarely.

Worse display if the user doesn't know about this. We should put this in a visible place in docs.

I think it's better to just have only one unconditional version of the dependency (current approach). This solution has some flaws, but at least simplifies a lot of things. But that's just my opinion.

🤔 Do I understand correctly that the approach taken in this PR is not in conflict with #771 because chrono types do not appear in the public API, even though the chrono dependency is used?

You are correct. We need the chrono dependency unconditionally for internal usage - to be exact, for std::fmt::Display for CqlValue.

OTOH, we expose chrono types in public API only when chrono-04 feature is enabled.

I wonder if we could prevent chrono from being used in public API unless the feature is activated.
One idea is to use cargo-public-api in CI, without chrono feature, and verify that chrono:: string is not present in the output.

muzarski · 2025-02-24T18:28:38Z

After moving displayer to scylla-cql, is it not possible to remove chrono dependency from scylla crate?

I checked it. We still use chrono in history.rs. I also tried disabling clock feature, but it seems to be required for https://docs.rs/chrono/latest/chrono/struct.DateTime.html#impl-From%3CSystemTime%3E-for-DateTime%3CLocal%3E.

muzarski · 2025-02-25T13:35:55Z

v1.1: Moved pretty module to src/pretty.rs

Lorak-mmk · 2025-02-25T17:16:02Z

@muzarski Please fix the conflict, then I'll merge.

Ref: https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format I used ", " (with whitespace) for all metadata/tracing related pretty-printing - we already did that for some tablet related logs, so let's be consistent in other logs. I left "," (without whitespace) for displaying CqlValue. There is a test for that, and I think it makes sense.

itertools::Format implements Debug: https://docs.rs/itertools/latest/itertools/structs/struct.Format.html#impl-Debug-for-Format%3C'a,+I%3E.

It will be needed once I move the pretty module to scylla-cql.

I'm not a big fan of it - tbh, I hate that I have to do that. But we need this dependency for unconditional internal usage after the next commit (once I move `CqlValueDisplayer` to scylla-cql). Any ideas how to avoid this? As of now, we won't get any compile warnings when someone exposes chrono-04 type in public API without "chrono-04" feature enabled. In addition, I enabled `alloc` feature for chrono-04. It's required for https://docs.rs/chrono/latest/chrono/struct.DateTime.html#method.format.

Implemented std::fmt::Display for CqlValue instead.

After all the work related to moving CqlValueDisplayer to scylla-cql, we can finally mark CqlValue as non_exhaustive.

muzarski · 2025-02-25T17:39:10Z

@muzarski Please fix the conflict, then I'll merge.

@Lorak-mmk done, and CI passed

muzarski self-assigned this Feb 24, 2025

muzarski added this to the 1.0.0 milestone Feb 24, 2025

muzarski marked this pull request as draft February 24, 2025 17:11

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Feb 24, 2025

muzarski force-pushed the cql-value-non-exhaustive branch 5 times, most recently from 0d9c84f to 6b02be4 Compare February 24, 2025 17:57

muzarski marked this pull request as ready for review February 24, 2025 18:05

muzarski requested review from wprzytula and Lorak-mmk February 24, 2025 18:05

muzarski added API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API labels Feb 24, 2025

muzarski mentioned this pull request Feb 24, 2025

result: mark ColumnType and related enums as non-exhaustive #1205

Merged

4 tasks

Lorak-mmk requested changes Feb 24, 2025

View reviewed changes

muzarski force-pushed the cql-value-non-exhaustive branch from 6b02be4 to ae8cc53 Compare February 25, 2025 13:35

muzarski requested a review from Lorak-mmk February 25, 2025 13:36

wprzytula previously approved these changes Feb 25, 2025

View reviewed changes

Lorak-mmk previously approved these changes Feb 25, 2025

View reviewed changes

muzarski added 6 commits February 25, 2025 18:19

pretty: remove DisplayUsingDebug

1230415

itertools::Format implements Debug: https://docs.rs/itertools/latest/itertools/structs/struct.Format.html#impl-Debug-for-Format%3C'a,+I%3E.

scylla: bump itertools dependency to latest 0.14.0

e934b9b

scylla-cql: add itertools dependency

5969b0a

It will be needed once I move the pretty module to scylla-cql.

pretty: move pretty module to scylla-cql

087a7ba

muzarski added 3 commits February 25, 2025 18:20

value: remove CqlValueDisplayer

9eaf4a9

Implemented std::fmt::Display for CqlValue instead.

value: deduplicate conversion logic for CqlDate and CqlTimestamp

f2c241c

value: mark CqlValue as non_exhaustive

7810380

After all the work related to moving CqlValueDisplayer to scylla-cql, we can finally mark CqlValue as non_exhaustive.

muzarski dismissed stale reviews from Lorak-mmk and wprzytula via 7810380 February 25, 2025 17:20

muzarski force-pushed the cql-value-non-exhaustive branch from ae8cc53 to 7810380 Compare February 25, 2025 17:20

Lorak-mmk approved these changes Feb 25, 2025

View reviewed changes

Lorak-mmk merged commit 370506e into scylladb:main Feb 25, 2025
11 checks passed

Lorak-mmk mentioned this pull request Mar 4, 2025

Release 1.0.0 #1265

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

value: mark CqlValue as non_exhaustive #1257

value: mark CqlValue as non_exhaustive #1257

muzarski commented Feb 24, 2025 •

edited

Loading

github-actions bot commented Feb 24, 2025 •

edited

Loading

Lorak-mmk left a comment

Lorak-mmk Feb 24, 2025 •

edited

Loading

Lorak-mmk Feb 24, 2025

muzarski Feb 25, 2025

wprzytula Feb 25, 2025

muzarski Feb 25, 2025

muzarski Feb 25, 2025

Lorak-mmk Feb 25, 2025

muzarski commented Feb 24, 2025

muzarski commented Feb 25, 2025

Lorak-mmk commented Feb 25, 2025

muzarski commented Feb 25, 2025

value: mark CqlValue as non_exhaustive #1257

value: mark CqlValue as non_exhaustive #1257

Conversation

muzarski commented Feb 24, 2025 • edited Loading

Motivation

Obstacles

Things to discuss

chrono 0.4 - Very important!!!

Pre-review checklist

github-actions bot commented Feb 24, 2025 • edited Loading

Lorak-mmk left a comment

Choose a reason for hiding this comment

Lorak-mmk Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Lorak-mmk Feb 24, 2025

Choose a reason for hiding this comment

muzarski Feb 25, 2025

Choose a reason for hiding this comment

wprzytula Feb 25, 2025

Choose a reason for hiding this comment

muzarski Feb 25, 2025

Choose a reason for hiding this comment

muzarski Feb 25, 2025

Choose a reason for hiding this comment

Lorak-mmk Feb 25, 2025

Choose a reason for hiding this comment

muzarski commented Feb 24, 2025

muzarski commented Feb 25, 2025

Lorak-mmk commented Feb 25, 2025

muzarski commented Feb 25, 2025

muzarski commented Feb 24, 2025 •

edited

Loading

github-actions bot commented Feb 24, 2025 •

edited

Loading

Lorak-mmk Feb 24, 2025 •

edited

Loading