-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: More Int128
testing and related fixes
#20494
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20494 +/- ##
==========================================
- Coverage 79.02% 79.02% -0.01%
==========================================
Files 1563 1563
Lines 220557 220609 +52
Branches 2492 2502 +10
==========================================
+ Hits 174300 174335 +35
- Misses 45684 45700 +16
- Partials 573 574 +1 ☔ View full report in Codecov by Sentry. |
crates/polars-python/src/map/mod.rs
Outdated
@@ -28,6 +28,7 @@ impl PyArrowPrimitiveType for Int8Type {} | |||
impl PyArrowPrimitiveType for Int16Type {} | |||
impl PyArrowPrimitiveType for Int32Type {} | |||
impl PyArrowPrimitiveType for Int64Type {} | |||
impl PyArrowPrimitiveType for Int128Type {} |
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.
This is maybe a misnomer, as Pyarrow doesn't support i128 primitive type.
Looking at this code, I also don't understand why we have this trait. Maybe we can remove it and replace the PyArrowPrimitiveType
with PolarsNumericType
.
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.
Yeah, I think PyArrowPrimitiveType
is a misnomer here. I renamed this to PyPolarsNumericType
.
I tried replacing it entirely with PolarsNumericType
as you suggested but couldn't figure out how to get around this build error:
error[E0119]: conflicting implementations of trait `ApplyLambda<'_>` for type `polars::prelude::ChunkedArray<polars::prelude::BooleanType>`
--> crates/polars-python/src/map/series.rs:571:1
|
280 | impl<'a> ApplyLambda<'a> for BooleanChunked {
| ------------------------------------------- first implementation here
...
571 | / impl<'a, T> ApplyLambda<'a> for ChunkedArray<T>
572 | | where
573 | | T: PolarsNumericType,
574 | | T::Native: IntoPyObject<'a> + FromPyObject<'a>,
575 | | ChunkedArray<T>: IntoSeries,
| |________________________________^ conflicting implementation for `polars::prelude::ChunkedArray<polars::prelude::BooleanType>`
|
= note: upstream crates may add a new impl of trait `polars::prelude::PolarsNumericType` for type `polars::prelude::BooleanType` in future versions
It appears there is a similar trait PolarsOpsNumericType
used in a similar way here:
trait PolarsOpsNumericType: PolarsNumericType {} |
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.
Ah, there we have the reason for this new trait. :)
Again, looks great. Thanks a lot @lukemanley |
No description provided.