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 support for some big number types #1014

Merged
merged 6 commits into from
Jan 2, 2024

Conversation

AkiraMiyakoda
Copy link
Contributor

This PR enables redis-rs to read/write some third-party "big number" types directly.

Motivation

We can store a number with any number of digits in Redis as a string. Although we can read such a value as a string and convert it into a desired numeric type, it's convenient when we can read it directly as a number like this:

use num_bigint::BigInt;

let v: BigInt = connection.get("foo").await?;

This library already works with Rust's intrinsic numeric types. This PR adds a feature called bignum to it so that it can handle some 'big number' types from some third-party crates which are mature and widely used.

What is this?

This PR contains FromRedisValue and ToRedisArgs implementation for following types:

  • rust_decimal::Decimal
  • bigdecimal::BigDecimal
  • num_bigint::BigInt
  • num_bigint::BigUint

It includes tests and a feature flag as well.

rust_decimal, bigdecimal and num-bigint are supported.
Copy link
Contributor

@nihohit nihohit left a comment

Choose a reason for hiding this comment

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

This looks mostly OK to me.
@jaymell since these are separate crates, do you think that these should be added as a single feature, or split between multiple features?

Makefile Outdated Show resolved Hide resolved
redis/tests/test_bignum.rs Outdated Show resolved Hide resolved
redis/src/bignum.rs Outdated Show resolved Hide resolved
redis/src/bignum.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nihohit nihohit left a comment

Choose a reason for hiding this comment

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

@jaymell LGMT, just not sure whether we want this as a single feature, or split the feature between crates, so that each bignum crate will have its own feature. WDYT?

@jaymell
Copy link
Contributor

jaymell commented Jan 2, 2024

This is great! But I definitely think it makes sense to not force everything into one feature. @AkiraMiyakoda Can we split this into multiple features, one for each external crate?

@AkiraMiyakoda
Copy link
Contributor Author

Hi @jaymell
Thanks for the review. I split the bignum feature into three which correspond to target crates.
New features' names are rust_decimal, bigdecimal and num-bigint just the same as the crates.

@nihohit nihohit merged commit 59e9a6a into redis-rs:main Jan 2, 2024
10 checks passed
@nihohit
Copy link
Contributor

nihohit commented Jan 2, 2024

Thanks for the contribution, and the excellent work!

@AkiraMiyakoda AkiraMiyakoda deleted the devel-big-numbers branch January 3, 2024 00:54
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