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

Hide public usages of bigdecimal::BigDecimal behind a crate feature #922

Merged
merged 7 commits into from Feb 1, 2024

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Jan 26, 2024

Refs: #771

Changes

  • introduced value::CqlDecimal type
  • hidden public usages of bigdecimal::Bigdecimal behind crate feature.
  • wrapped CqlDecimal with CqlValue::Decimal
  • updated docs
  • updated tests
  • added docstrings

Points to discuss

  • how to implement PartialEq for CqlDecimal? Note that CqlDecimal makes use of CqlVarint, so the bytes normalization part can be delegated to CqlVarint. However, there is an additional issue, because there are multiple pairs (integer, scale) which can represent the same number. Take for example: 1e3 and 100e1. I don't think it's trivial to implement the comparison without converting bytes (hexadecimal representation) to decimal number.
  • Should I create some macros for code repetitions? There are some parts of code that are repeated for each version of the bigdecimal crate that unfortunately cannot be expressed using some generic function (e.g. usages of BigDecimal::new() constructor).

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.

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/result.rs Outdated Show resolved Hide resolved
docs/source/data-types/decimal.md Outdated Show resolved Hide resolved
@Lorak-mmk
Copy link
Collaborator

* how to implement `PartialEq` for `CqlDecimal`? Note that `CqlDecimal` makes use of `CqlVarint`, so the bytes normalization part can be delegated to `CqlVarint`. However, there is an additional issue, because there are multiple pairs (integer, scale) which can represent the same number. Take for example: `1e3` and `100e1`. I don't think it's trivial to implement the comparison without converting bytes (hexadecimal representation) to decimal number.

I guess it would be useful for users who don't need BigDecimal. This change should be backwards compatible so if implementing it is too hard, I think we can open an issue about it and skip to for now.
How does BigDecimal implement it?

* Should I create some macros for code repetitions? There are some parts of code that are repeated for each version of the `bigdecimal` crate that unfortunately cannot be expressed using some generic function (e.g. usages of `BigDecimal::new()` constructor).

I don't think it's important right now. We can do this in the future when repetitions become problematic.

examples/Cargo.toml Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla/src/utils/pretty.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Show resolved Hide resolved
@piodul
Copy link
Collaborator

piodul commented Jan 31, 2024

  • how to implement PartialEq for CqlDecimal? Note that CqlDecimal makes use of CqlVarint, so the bytes normalization part can be delegated to CqlVarint. However, there is an additional issue, because there are multiple pairs (integer, scale) which can represent the same number. Take for example: 1e3 and 100e1. I don't think it's trivial to implement the comparison without converting bytes (hexadecimal representation) to decimal number.

It's not terribly difficult, but not simple either. I'm looking at the BigDecimal's implementation right now and it looks like you would have to implement some arithmetic on CqlVarint - at least addition and multiplication. Implementing something like this naively isn't an insurmountable task and can be a fun exercise, but would introduce quite a bit of complex code just to implement one thing for which it isn't clear if it's useful at all. I also vote to skip this for now and consider implementing it if somebody asks.

  • Should I create some macros for code repetitions? There are some parts of code that are repeated for each version of the bigdecimal crate that unfortunately cannot be expressed using some generic function (e.g. usages of BigDecimal::new() constructor).

As I wrote in review, I think we should consider dropping support for 0.2 and 0.3 for simplicity. I suspect that macros will not be necessary anymore after that.

@muzarski
Copy link
Contributor Author

v2: addressed review comments, apart from the comment regarding removing support for bigdecimal v0.2 and v0.3. Waiting for @Lorak-mmk opinion on this topic.

@piodul
Copy link
Collaborator

piodul commented Jan 31, 2024

By the way, you need to update Cargo.lock.msrv. There is an instruction by @Lorak-mmk in CONTRIBUTING.md on how to do that.

@muzarski
Copy link
Contributor Author

v3: removed support for bigdecimal v0.2 and v0.3

@muzarski
Copy link
Contributor Author

v4: updated Cargo.lock.msrv file

Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

I think that in general bumping MSRV should be done in the same commit where Cargo.lock is updated - technically speaking, the change is not bisectable under min_rust job, but I really doubt that anybody will do something like that so that is a non-issue.

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla/src/utils/pretty.rs Outdated Show resolved Hide resolved
@muzarski
Copy link
Contributor Author

v5: addressed review comments

@muzarski muzarski requested a review from piodul January 31, 2024 18:47
@Lorak-mmk
Copy link
Collaborator

I think that in general bumping MSRV should be done in the same commit where Cargo.lock is updated - technically speaking, the change is not bisectable under min_rust job, but I really doubt that anybody will do something like that so that is a non-issue.

Maybe it's not important for bisectability, but I'd still like those commits to be squashed. Updating Cargo.lock isn't an unrelated change in this case, it is a required change because of .toml modification.
Also in the commits that bump our packages versions we update .toml and .lock in 1 commit so it would be good to be consistent here.

@muzarski
Copy link
Contributor Author

v6: squashed Cargo.lock.msrv commit with bigdecimal version bump commit

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

v7: removed mention about bigdecimal v0.2 and v0.3 from the docs

Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

One last thing - I don't think it fixes #771 entirely.

First and foremost, the chrono, time and secrecy features enable dependencies that do not have stable releases, but their names do not have the version of those dependencies in them.

Second, the issue is not just about types used for serialization. We have other pre-1.0 dependencies which may prevent us from stabilizing the API:

  • histogram - our Metrics type contains a Histogram. We should not expose it publicly but rather reexport its functionality through Metrics' methods.
  • num_enum - it defines a bunch of traits and we implement them for some of our public types (and num_enum::TryFromPrimitiveError appears in our FrameError). We could easily implement the code that the crate generates automatically ourselves.
  • openssl - it's optional right now but the crates themselves are not stable. While those are only bindings to OpenSSL and are compatible with a wide range of versions (in case of OpenSSL they seem to be forwards-compatible), the openssl crate itself might turn out to have some security issues and maybe we will have to bump its version.
  • strum, strum_macros - I believe they have a similar problem to num_enum and the solution is the same for them.
  • async-trait - we use it for some of our public traits (AddressTranslator, AuthenticatorProvider, AuthenticatorSession). We may consider switching to recently stabilized async traits or change the methods to return boxed futures.

There were also some concerns about other, >=1.0 dependencies.

scylla-cql/src/frame/value.rs Show resolved Hide resolved
docs/source/data-types/decimal.md Outdated Show resolved Hide resolved
@piodul
Copy link
Collaborator

piodul commented Feb 1, 2024

hidden public usages of bigdecimal::Bigdecimal behind crate features. Available versions of bigdecimal crates are: 0.2.0, 0.3, 0.4

The PR description is also outdated

Before this commit, we wouldn't check for the value overflow
when computing the bigdecimal serialized bytes length.

After this commit, we make use of explicit overflow checks before
performing the addition.
@piodul piodul merged commit e94f8f9 into scylladb:main Feb 1, 2024
9 checks passed
@Lorak-mmk Lorak-mmk mentioned this pull request Feb 1, 2024
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.

None yet

3 participants