Skip to content

chore: Fix Decimal precision annotation#25227

Merged
alexander-beedie merged 1 commit intopola-rs:mainfrom
vyasr:fix/precision_annotation
Nov 10, 2025
Merged

chore: Fix Decimal precision annotation#25227
alexander-beedie merged 1 commit intopola-rs:mainfrom
vyasr:fix/precision_annotation

Conversation

@vyasr
Copy link
Copy Markdown
Contributor

@vyasr vyasr commented Nov 8, 2025

On construction the precision if set to None is shifted to 38, so the precision is always non-None.

@vyasr vyasr changed the title Fix Decimal precision annotation chore: Fix Decimal precision annotation Nov 8, 2025
@github-actions github-actions Bot added internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars and removed title needs formatting labels Nov 8, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.89%. Comparing base (a3474bb) to head (ba95966).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25227      +/-   ##
==========================================
+ Coverage   81.77%   81.89%   +0.11%     
==========================================
  Files        1713     1714       +1     
  Lines      236780   239028    +2248     
  Branches     3013     3249     +236     
==========================================
+ Hits       193629   195745    +2116     
- Misses      42381    42485     +104     
- Partials      770      798      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alexander-beedie
Copy link
Copy Markdown
Collaborator

alexander-beedie commented Nov 8, 2025

Thanks for contributing - however, as per the docstring, precision=None means "inferred". So, while any eventual DataFrame will indeed be initialised with a specific (inferred) Decimal precision (that will typically be 38), it is fine for the dtype itself to have precision=None. The typing as-is looks correct to me.

pl.Decimal(precision=None, scale=3)
# Decimal(precision=None, scale=3)

vyasr added a commit to vyasr/cudf that referenced this pull request Nov 8, 2025
Add assertion and cast for dtype.precision in _dtype_to_header to work around
incorrect polars type stubs where precision is typed as int | None. At runtime,
Decimal precision is never None.

This workaround is gated by POLARS_VERSION_LT_136 check and can be removed when
upgrading to polars >= 1.36, which will include the upstream fix.

Upstream fix: pola-rs/polars#25227

Reduces mypy errors from 31 to 30.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@TomAugspurger
Copy link
Copy Markdown

TomAugspurger commented Nov 10, 2025

@alexander-beedie are we looking at different versions? I see this:

In [1]: import polars as pl

In [2]: pl.Decimal(precision=None, scale=3)
Out[2]: Decimal(precision=38, scale=3)

In [3]: pl.__version__
Out[3]: '1.35.2'

It looks like #24742 might have changed the docstring, but not the type annotation.

@alexander-beedie
Copy link
Copy Markdown
Collaborator

alexander-beedie commented Nov 10, 2025

Oof; I stand corrected - apparently I was looking at this on a secondary laptop that hadn't been updated to latest main 🤦‍♂️ Reopening and will merge shortly - thanks for catching me on that!

@alexander-beedie alexander-beedie merged commit 16073b0 into pola-rs:main Nov 10, 2025
40 of 41 checks passed
@coastalwhite
Copy link
Copy Markdown
Collaborator

coastalwhite commented Nov 10, 2025

I don't think this is correct. None now means maximum, which is 38. As decimals were stabilized last release, this would be a breaking change.

@coastalwhite
Copy link
Copy Markdown
Collaborator

Nevermind, this only touches the member variables and not the constructor.

@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented Nov 10, 2025

@alexander-beedie thanks for reviewing so quickly! However, I think the behavior you're showing might be outdated. #24542 modified how precision works for decimals (specifically this diff), see the extended discussion in #19784. As a result precision is always set to a non-None value (of 38) on construction if None is provided. Consider:

❯ uv pip install polars==1.33
...
❯ python -c "import polars as pl; print(pl.Decimal(precision=None, scale=3))"
Decimal(precision=None, scale=3)
❯ uv pip install polars==1.34
...
❯ python -c "import polars as pl; print(pl.Decimal(precision=None, scale=3))"

@vyasr
Copy link
Copy Markdown
Contributor Author

vyasr commented Nov 10, 2025

Ah my browser just refreshed and I see multiple new comments and that you all already figured this out as well and merged the PR, so we're all set. Thanks all!

@vyasr vyasr deleted the fix/precision_annotation branch November 10, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement python Related to Python Polars rust Related to Rust Polars

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants