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 get_decimal() #177

Merged
merged 7 commits into from Jan 4, 2024
Merged

Add get_decimal() #177

merged 7 commits into from Jan 4, 2024

Conversation

M5135213521asd3
Copy link
Contributor

Get cassadra decimal value as BigDecimal (https://docs.rs/bigdecimal/latest/bigdecimal/)
Signed-off-by: Marian Marencik github@marianm.net

Get cassadra decimal value as BigDecimal (https://docs.rs/bigdecimal/latest/bigdecimal/)
Signed-off-by: Marian Marencik <github@marianm.net>
Copy link
Contributor Author

@M5135213521asd3 M5135213521asd3 left a comment

Choose a reason for hiding this comment

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

Signed-off-by: Marian Marencik github@marianm.net

Signed-off-by: Marian Marencik github@marianm.net
Copy link
Collaborator

@kw217 kw217 left a comment

Choose a reason for hiding this comment

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

Thanks @M5135213521asd3 , this looks good. Just a couple of things:

  • Please can you ensure you return the actual error, rather than a fixed one.
  • I think this must be missing the code to supply a decimal value to Cassandra (i.e., bind). Please can you add that?
  • Please can you add a decimal test in all the relevant places, to tests/basic.rs? Use a value that requires multiple bytes and a non-zero scale, so that the code is properly exercised.
  • Please can you also add a specific test for endianness (to the same file), which uses a literal CQL statement to write a decimal value (e.g., INSERT INTO blah ... VALUES (99999.1234)), and then uses get_decimal to retrieve it and checks it gets the right value?

Thanks!

src/cassandra/value.rs Outdated Show resolved Hide resolved
src/cassandra/value.rs Outdated Show resolved Hide resolved
M5135213521asd3 added 4 commits December 19, 2023 20:17
Signed-off-by: Marian Marencik github@marianm.net
fmt
Signed-off-by: Marian Marencik github@marianm.net
Signed-off-by: Marian Marencik github@marianm.net
Signed-off-by: Marian Marencik github@marianm.net
@M5135213521asd3
Copy link
Contributor Author

  • I think this must be missing the code to supply a decimal value to Cassandra (i.e., bind). Please can you add that?

I have include it to this request, but from my side there is one todo:

//@TODO: cpp scale = i32, BigInt scale = i64
        unsafe {
            cass_statement_bind_decimal(
                self.inner(),
                index,
                varint.as_ptr(),
                varint.len(),
                **scale as i32,**
            )
            .to_result(self)

Copy link
Collaborator

@kw217 kw217 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 very much! This is looking great! I have a suggestion for how to deal with your remaining TODO; otherwise this is fine!

let varint = dec_parts.0.to_signed_bytes_be();
let scale = dec_parts.1;

//@TODO: cpp scale = i32, BigInt scale = i64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I think it's highly unlikely anyone will be using 4GB BigInts, but just in case, you should convert scale using try_into() and then if it fails return an error. I think it's OK to reuse one of the CASS_ERROR_LIB errors here, even though that's not strictly accurate: probably CASS_ERROR_LIB_INVALID_DATA.

let varint = dec_parts.0.to_signed_bytes_be();
let scale = dec_parts.1;

//@TODO: cpp scale = i32, BigInt scale = i64
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above.

Signed-off-by: Marian Marencik github@marianm.net
Copy link
Collaborator

@kw217 kw217 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@kw217 kw217 merged commit 9698afb into Metaswitch:main Jan 4, 2024
4 checks passed
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

2 participants