Skip to content
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): start further use of polars-compute in polars-parquet #16788

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

coastalwhite
Copy link
Collaborator

This adds further usage of polars-compute in polars-parquet. I cannot get it to work for Primitives since there is a very, very annoying trait bound I cannot really circumvent without changing polars-compute.

@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars labels Jun 6, 2024
Copy link

codspeed-hq bot commented Jun 6, 2024

CodSpeed Performance Report

Merging #16788 will degrade performances by 16.26%

Comparing coastalwhite:use-polars-compute-in-parquet (b6ae87a) with main (b329894)

Summary

❌ 1 regressions
✅ 36 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main coastalwhite:use-polars-compute-in-parquet Change
test_tpch_q2 4.5 ms 5.3 ms -16.26%

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.34%. Comparing base (4d35be2) to head (b6ae87a).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16788      +/-   ##
==========================================
+ Coverage   81.29%   81.34%   +0.04%     
==========================================
  Files        1424     1424              
  Lines      187205   187165      -40     
  Branches     2714     2701      -13     
==========================================
+ Hits       152194   152244      +50     
+ Misses      34514    34425      -89     
+ Partials      497      496       -1     

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

@ritchie46
Copy link
Member

Got a repro of this trait bound issue?

@coastalwhite
Copy link
Collaborator Author

Essentially, trying to use the MinMaxKernel from polars-compute for the arrow::NativeType, will only work if simd is disabled. When simd is enabled, implementations of the MinMaxKernel are not done over NativeType but over specific types.

Even trying to include something like PrimitiveArray<T>: for<'a> MinMaxKernel<Scalar<'a> = T> will not solve the issue, since you will run into an issue where eventually the NativeType essentially needs to implement MinMaxKernel again. No real solution for now, will look at it later again.

@ritchie46
Copy link
Member

Alright, let's look at that one later.

@ritchie46 ritchie46 merged commit 2e633dc into pola-rs:main Jun 7, 2024
22 checks passed
@c-peters c-peters added the accepted Ready for implementation label Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation internal An internal refactor or improvement rust Related to Rust Polars
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants