refactor: Simplify _integer_fits_in_decimal; disallow supercasting for Datetime and Duration with different time_unit's#3526
Conversation
dangotbanned
left a comment
There was a problem hiding this comment.
Thanks @FBruzzesi
Just reviewed the Decimal part for now, thanks for taking that on board
I had an idea for TimeUnit but ran of time to explain 😂
| else: | ||
| bits: int = integer._bits | ||
| if isinstance(integer, SignedIntegerType): | ||
| bits = bits - 1 | ||
|
|
||
| value = (1 << bits) - 1 |
| def _integer_fits_in_decimal(value: int, precision: int, scale: int) -> bool: | ||
| """Scales an integer and checks if it fits the target precision.""" | ||
| # !NOTE: Indexing is safe since `scale <= precision <= 38` | ||
| return (precision == DEC128_MAX_PREC) or ( | ||
| value * POW10_LIST[scale] < POW10_LIST[precision] | ||
| ) | ||
| return (precision == DEC128_MAX_PREC) or (value * (10**scale) < (10**precision)) |
There was a problem hiding this comment.
(1) Could we be lazy-er?
I would prefer if we defer generating this until it is needed.
E.g. I'd expect
_integer_supertypingand_primitive_numeric_supertypingto be more commonly used - but even they don't exist at module-import-time
Sorry for not being clear here!
I think you are right to pre-compute these numbers.
Edit: ffs, reading this back really looks like AI.
Gonna leave it as-is, but I promise I wrote that 😭
How big are they?
AFAICT, these are all the possible inputs each parameter can have.
value: Literal[127, ..., 340282366920938463463374607431768211455]
# ^^^ 8 hidden values
precision: Literal[1, ..., 38]
scale: Literal[1, ..., 38] # TIL: polars seems to allow `0`So this makes the worst-case 😨:
(
34028236692093846346337460743176821145500000000000000000000000000000000000000
< 100000000000000000000000000000000000000
)Suggestion
Show _{integer,primitive_numeric}_supertyping
narwhals/narwhals/dtypes/_supertyping.py
Lines 118 to 153 in 23cc3e4
Iff we need it, the result is cached and then we reuse from there 🙂
So the suggestion was just to move the global dict/list into a function
|
|
||
| def _integer_fits_in_decimal(value: int, precision: int, scale: int) -> bool: | ||
| """Scales an integer and checks if it fits the target precision.""" | ||
| # !NOTE: Indexing is safe since `scale <= precision <= 38` |
There was a problem hiding this comment.
# !NOTE: Indexing is safe since scale <= precision <= 38
I suppose this comment may be relevant again depending on (#3526 (comment))


Description
This PR has two commits:
_integer_fits_in_decimalfunctionget_supertype#3396 (review) to disallow supercasting forDatetimeandDurationwith differenttime_unit's. I hope the explanation in the documentation is ok as well.What type of PR is this? (check all applicable)
Checklist