- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
perf: Elide the total order wrapper for non-(float/option) types #14648
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14648 +/- ##
==========================================
- Coverage 80.78% 80.77% -0.02%
==========================================
Files 1326 1326
Lines 173157 173227 +70
Branches 2453 2453
==========================================
+ Hits 139891 139923 +32
- Misses 32792 32830 +38
Partials 474 474 ☔ View full report in Codecov by Sentry. |
…ption<TotalOrdWrap<T>>`
@@ -36,7 +37,7 @@ pub trait PolarsObjectSafe: Any + Debug + Send + Sync + Display { | |||
|
|||
/// Values need to implement this so that they can be stored into a Series and DataFrame | |||
pub trait PolarsObject: | |||
Any + Debug + Clone + Send + Sync + Default + Display + TotalHash + TotalEq | |||
Any + Debug + Clone + Send + Sync + Default + Display + Hash + TotalHash + PartialEq + Eq + TotalEq |
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.
wasn't straightforward to add ToTotalOrd
trait for vec_hash
on PolarsObject
so juts hash directly for now, since I don't think it's needed
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.
Indeed. Objects are 🤷
@@ -185,8 +188,7 @@ where | |||
let group_probe_table = unsafe { | |||
hash_tbls.get_unchecked(hash_to_partition(by_left_k.dirty_hash(), n_tables)) | |||
}; | |||
let Some(right_grp_idxs) = group_probe_table.get(&TotalOrdWrap(*by_left_k)) | |||
else { | |||
let Some(right_grp_idxs) = group_probe_table.get(by_left_k) else { |
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.
Because the build_tables
function now uses .to_total_ord()
this is now just a plain BytesHash
instead of a TotalOrdWrap<BytesHash>
, so we don't need to call .to_total_ord()
here (this function is asof_join_by_binary
).
Thank you @nameexhaustion. |
This should be a slight perf improvement for hashing primitive types, but more importantly will revert the change in the hash output values for integer types introduced by #14617 (see #14617 (comment)).
Benchmark