-
Notifications
You must be signed in to change notification settings - Fork 176
feat: Improve support for Decimal DType
#3377
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
base: main
Are you sure you want to change the base?
Conversation
|
Regarding the stable API, how do we feel for this feature? I would say that keeping it backward compatible for v2 might be a stretch, but for v1 it would be ok. cc @MarcoGorelli |
|
thanks for your pr! i was under the impression that |
To save you a long read, there was nothing conclusive in that issue. On the Python-side, all that's changed is There are a lot more changes on the Rust-side (who would've guessed that?) I'm not pitching we do/don't do that - just trying to summarize the old-fashioned way π |
MarcoGorelli
left a comment
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.
thanks @FBruzzesi !
narwhals/dtypes.py
Outdated
| def __init__(self, precision: int | None = None, scale: int = 0) -> None: | ||
| self.precision = 38 if precision is None else precision |
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.
not sure what i'm missing but why not
precision: int = 38
?
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.
I suppose if we got an old pl.Decimal, that could have a None already?
Otherwise - yeah if we can avoid introducing the contructor with a None - I'm on board π
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.
It's solely to mirror polars signature: polars.datatypes.Decimal and code
def __init__(
self,
precision: int | None = None,
scale: int = 0,
) -> None:
if precision is None:
precision = 38
self.precision = precision
self.scale = scale|
@MarcoGorelli thanks for reviewing this. I have a couple of questions that might need to be discussed before merging:
Example: import polars as pl
dtype = pl.Decimal(2, 3) # Ok so far
pl.Series([1.2]).cast(dtype)
> InvalidOperationError: scale must be less than or equal to precision |
|
RE: (#3377 (comment)), (#3377 (comment)) @MarcoGorelli did you mean to introduce it as stable in #1571? |
@FBruzzesi I agree, let's do that Note Sorry for blowing up CI with |
Spotted while reviewing #3377 Pretty sure there's more of these to be found

Description
Adds
precisionandscaleparameters toDecimaldtype.TODO: Decide on stable API
What type of PR is this? (check all applicable)
Related issues
precisionandscalekeywords forDecimaldtypeΒ FBruzzesi/anyschema#101Checklist