I created the PR as suggested by @davidhewitt in #2774 to start the conversation. I added an initial benchmark but I'm not sure if it's sufficient to start. The results were:

Benchmarking decimal_via_extract: Collecting 100 samples in estimated 5.0001 s (
decimal_via_extract     time:   [46.788 ns 46.803 ns 46.819 ns]
Found 15 outliers among 100 measurements (15.00%)
  7 (7.00%) low severe
  5 (5.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

not sure why I had that trailing open paren.

The tests fail because I haven't written any docs yet. I've still got to address @adamreichold's feedback as well but I figured I'd start with the benchmarks to see if it makes sense and if not talk to CPython upstream about exposing an API. If the benchmarks are okay then I'll work in parallel. Add the necessary bits to raise this to completion while getting an API exposed in CPython.

Personally, I think that independently of the kind of (modernish) system this was run on, that the performance is reasonable and would have this than have it not, understanding that we can always improve things if better CPython API become available.

IMO we should only have these conversions if they're better than what a user could naively write themselves. Python's decimal has no C api, so this isn't the case here.

IMO we should only have these conversions if they're better than what a user could naively write themselves. Python's decimal has no C api, so this isn't the case here.

But why force the user into reinventing this if we can solve the problem once for everybody? As long as it is the best possible solution. And if a better way comes along, we can improve things in a single place and everyone benefits immediately.

@mejrs My thought process for this PR was that decimal is part of the stdlib as a type. I don't think it makes sense to add every part of stdlib but looking through most of the types (I'm counting the types under the numerical as well) are covered except for big integers which is what this aims to cover.

I'm not actively catching up on the project, so I'm not sure, but is there any other module where we support conversion between some Rust library and Python object which doesn't have C-API? If so, having this would be OK.

But I'm not very convinced with this string-based conversion... If we can use tuple instead, it would be faster.

IMO we should only have these conversions if they're better than what a user could naively write themselves. Python's decimal has no C api, so this isn't the case here.

I keep going back and forth on this line of thinking. I think ultimately the types with a C-API are not significantly different except that we wrap away the unsafe parts.

I do agree with @adamreichold's argument that getting this once right here is better for the ecosystem than forcing fragmentation. Also we essentially use Python APIs for PySequence and PyMapping now that we use the collections base classes, so perhaps we have to accept that to make "nice" bindings between the two languages we have to accept the Python C-API limitations at times?

Although I still agree it would be great to have a native conversion provided upstream, if there is willingness.

But I'm not very convinced with this string-based conversion... If we can use tuple instead, it would be faster.

It's not clear to me which conversion options are available, however it would be good to sense-check what we're offering here is the most efficient.

Also of note is that we do have similar precedent with support for converting pathlib.Path into Rust types.

So the reason I went with the string over the tuple is that the Python tuple doesn't map to the Rust interface nicely.

The Python tuple documented at https://docs.python.org/3/library/decimal.html#decimal.Decimal says its

If value is a tuple, it should have three components, a sign (0 for positive or 1 for negative), a tuple of digits, and an integer exponent. For example, Decimal((0, (1, 4, 1, 4), -3)) returns Decimal('1.414')
which would be returned from https://docs.python.org/3/library/decimal.html#decimal.Decimal.as_tuple

We'd have a couple of weird corner cases with Infinity and NaN via as_tuple as well.

>>> nan = Decimal('NaN')
>>> nan.as_tuple()
DecimalTuple(sign=0, digits=(), exponent='n')
>>> inf = Decimal('Infinity')
>>> inf.as_tuple()
DecimalTuple(sign=0, digits=(0,), exponent='F')

Now from the Rust side the closest to the tuple interface is probably new() or from_str_radix(). https://docs.rs/rust_decimal/1.28.1/rust_decimal/struct.Decimal.html#method.new and https://docs.rs/rust_decimal/1.28.1/rust_decimal/struct.Decimal.html#method.from_str_radix

• new() takes in the digits as a signed number and the exponent. So that will require some conversion of the digits to an i64
• from_str_radix() would take the digits as a &str and would probably be easier to convert than the above but will cause a temp allocation. But this would probably perform better in the Rust side than the Python side.

So ultimately if that's approach you'd like me to take I think I'd steer towards from_str_radix() if that sounds okay?

@cardoe
Thank you for your explanation. String-based conversion sounds reasonable for me now.

I believe this addresses all the feedback thus far and could be the first commit considered for a full review / inclusion.

I think this fixed the MSRV issue.

I've just got an M1 based Mac to test on right now so the MSRV won't build on here so fingers crossed really.

Hello, I'll do my best to review this tomorrow. A bit behind on OSS this week, had a sick family. We're better now finally :)

Hello, I'll do my best to review this tomorrow. A bit behind on OSS this week, had a sick family. We're better now finally :)

Take you time. Family first. OSS is the shoulder monkey that never goes away for better or worse. I'm not in any rush on this. I was playing around with a Python code base and grumping at "type whispering" biting me on something and though I would try to lift part of the code base into Rust and looked for Decimal didn't see it. So I wrote a wrapper type for my use case but figured since there was at least one other person looking for it that I would try to contribute back to make it easier for the next person.

First time contributor has agreed to the new licensing scheme.

I believe I've addressed all the PR feedback. Please let me know what I can do going forward. The current GitHub actions failures are happening for me locally on my machine with the latest main commit as well.

@cardoe Could you rebase on main now that #3114 has landed?

bors r+

Build failed:

• conclusion

bors retry

Build succeeded:

• conclusion