-
Notifications
You must be signed in to change notification settings - Fork 236
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 decimal validator #763
Conversation
CodSpeed Performance ReportMerging #763 will degrade performances by 43.74%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a good start.
The easiest way to check performance would be to copy the validator constructed by pydantic and add it to benchmarks here, together with the function from pydantic. Then run benchmarks. You can probably remove that once we're sure it's faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple more points.
e8254f0
to
5bb39f3
Compare
I think the implementation here for the validator is now complete and is as optimized as can be. Still to do:
I will park this until we make a decision whether to move forwards with it. The alternative is pydantic/pydantic#6781 |
1586486
to
a088741
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #763 +/- ##
==========================================
- Coverage 94.06% 93.82% -0.24%
==========================================
Files 102 104 +2
Lines 15023 15365 +342
Branches 25 25
==========================================
+ Hits 14131 14416 +285
- Misses 886 943 +57
Partials 6 6
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
ed0bd96
to
2224e98
Compare
2fa96b3
to
0f7ca0d
Compare
input: &'a impl Input<'a>, | ||
decimal_type: &'a PyType, | ||
) -> ValResult<'a, &'a PyAny> { | ||
decimal_type.call1((arg,)).map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning this part maybe we can do the calculation in rust and set the attributes of the python class Decimal
: __slots__ = ('_exp','_int','_sign', '_is_special')
WDYT @davidhewitt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR I'm trying to keep the same logic as pydantic
just move the implementation. I would definitely be interested in this as a future improvement. I believe @samuelcolvin had some previous thoughts on how the parsing might be done (by looking at the digits as a string), not sure if there was any implementation put somewhere?
9b714c9
to
0b08d2c
Compare
fn strict_decimal(&'a self, decimal_type: &'a PyType) -> ValResult<&'a PyAny> { | ||
let py = decimal_type.py(); | ||
match self { | ||
JsonInput::Float(f) => create_decimal(PyString::new(py, &f.to_string()), self, decimal_type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for us to make sure that when parsing numeric JSON into a decimal it is treated as a string rather than loaded into an f64 or whatever? I think @samuelcolvin said he did something like this in speedate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Maybe it's already doing that, not sure, but it's probably worth checking that we load many-digit JSON numbers into decimal properly)
tests/serializers/test_decimal.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me worth adding a test that Decimal('123456789123456789123456789.123456789123456789123456789')
(or some other similarly high-precision decimal) gets serialized properly (at least to JSON).
I made various comments; I think at least some merit changes (e.g., removing the print line), and I do think there are some logic changes worth making (i.e., allowing proper subclasses of For hooky: please update |
Thanks for the thorough review; that definitely caught a couple of bugs! I've rebased this PR and added a commit to make the desired review changes. |
please review |
Many thanks, I rebased and dropped the commit which pointed the integration tests at pydantic/pydantic#6858 👍 |
Change Summary
Adds a Rust implementation of decimal validation and serialization. Needs more tests and benchmarks to confirm it's worth going for this instead of pydantic/pydantic#6327
Related issue number
N/A
Checklist
pydantic-core
(except for expected changes)Selected Reviewer: @dmontagu