-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
refactor(rust): More refactor for PlSmallStr #18456
Conversation
} | ||
} | ||
|
||
impl core::fmt::Display for PlSmallStr { | ||
#[inline(always)] | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
self.0.fmt(f) | ||
self.as_str().fmt(f) |
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.
Ensure we use the Display
impl from &str
impl core::fmt::Debug for PlSmallStr { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
self.as_str().fmt(f) | ||
impl From<&&str> for PlSmallStr { |
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.
When do we have double indirection in &&str
?, I think we should just deref.
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.
Yes, please don't add this.
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.
We have some functions that take impl IntoIterator<Item = impl Into<PlSmallStr>>
-
polars/crates/polars-ops/src/frame/join/mod.rs
Lines 84 to 93 in b3172aa
fn join<I, S>( | |
&self, | |
other: &DataFrame, | |
left_on: I, | |
right_on: I, | |
args: JoinArgs, | |
) -> PolarsResult<DataFrame> | |
where | |
I: IntoIterator<Item = S>, | |
S: Into<PlSmallStr>, |
and they get passed &[&str]
-
polars/crates/polars/tests/it/core/joins.rs
Lines 508 to 515 in b3172aa
let df_inner_join = df_left | |
.join( | |
&df_right, | |
&["col1", "join_col2"], | |
&["join_col1", "col2"], | |
JoinType::Inner.into(), | |
) | |
.unwrap(); |
Whose IntoIterator
then becomes &&str
, so we needed the From<&&str>
.
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 think we can just pass ["col1", "join_col2"]
instead of &["col1", "join_col2"]
.
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.
That still gives *nvm: this has changed since a while ago in Rust&&str
for the IntoIter
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.
@ritchie46 removing this causes a huge amount of breakage, I will do it in a later PR
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18456 +/- ##
==========================================
- Coverage 79.84% 79.83% -0.01%
==========================================
Files 1497 1498 +1
Lines 201828 201803 -25
Branches 2867 2867
==========================================
- Hits 161141 161115 -26
- Misses 40141 40142 +1
Partials 546 546 ☔ View full report in Codecov by Sentry. |
&self, | ||
other: &DataFrame, | ||
left_on: impl IntoIterator<Item = impl Into<PlSmallStr>>, | ||
right_on: impl IntoIterator<Item = impl Into<PlSmallStr>>, |
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.
pre-work for removing From<&&str>
- as then calling join(left_on=["a"], right_on=["a", "b"])
becomes 2 distinct generics <[&str; 1] as IntoIterator>
, <[&str; 2] as IntoIterator>
*although also in general, allows for 2 distinct types
.as_ref().into()
->.clone()
Removes*breaks a lot of things - will do separatelyFrom<&&str>
:Existing code calling with constant arrays are changed from e.g.Series::new(_, &["a", ...])
toSeries::new(_, ["a", ...])
&[&str]
can no longer be passed to most API functions acceptingimpl IntoIterator<Item = impl Into<PlSmallStr>>
AsRef<Path>
,AsRef<[u8]>
,AsRef<OsStr>