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

[CHORE]: prepare for nulls first/last kernels #3301

Merged
merged 7 commits into from
Nov 20, 2024

Conversation

universalmind303
Copy link
Contributor

@universalmind303 universalmind303 commented Nov 15, 2024

This pr adds nulls_first to all sort functions in preparation for implementing the nulls first/last kernels. This will be followed up by adding the actual nulls first/last implementations.

Copy link

codspeed-hq bot commented Nov 15, 2024

CodSpeed Performance Report

Merging #3301 will improve performances by 67.12%

Comparing universalmind303:nulls-first (017c480) with main (1b84250)

Summary

⚡ 1 improvements
✅ 16 untouched benchmarks

Benchmarks breakdown

Benchmark main universalmind303:nulls-first Change
test_iter_rows_first_row[100 Small Files] 312.1 ms 186.8 ms +67.12%

src/daft-core/src/array/ops/sort.rs Outdated Show resolved Hide resolved
src/daft-logical-plan/src/ops/project.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 73.01980% with 109 lines in your changes missing coverage. Please review.

Project coverage is 77.31%. Comparing base (0709691) to head (017c480).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-core/src/array/ops/sort.rs 59.42% 28 Missing ⚠️
daft/table/table.py 9.52% 19 Missing ⚠️
src/daft-table/src/python.rs 0.00% 12 Missing ⚠️
daft/table/micropartition.py 54.54% 10 Missing ⚠️
src/daft-sql/src/planner.rs 77.27% 10 Missing ⚠️
src/daft-physical-plan/src/ops/sort.rs 20.00% 8 Missing ⚠️
src/daft-core/src/utils/mod.rs 64.70% 6 Missing ⚠️
src/daft-table/src/ops/groups.rs 0.00% 5 Missing ⚠️
daft/expressions/expressions.py 66.66% 2 Missing ⚠️
daft/series.py 85.71% 2 Missing ⚠️
... and 6 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3301      +/-   ##
==========================================
- Coverage   77.59%   77.31%   -0.29%     
==========================================
  Files         668      678      +10     
  Lines       82211    83063     +852     
==========================================
+ Hits        63793    64218     +425     
- Misses      18418    18845     +427     
Files with missing lines Coverage Δ
daft/dataframe/dataframe.py 86.65% <100.00%> (ø)
daft/execution/execution_step.py 89.47% <100.00%> (+0.03%) ⬆️
daft/execution/physical_plan.py 94.01% <ø> (ø)
daft/execution/rust_physical_plan_shim.py 95.60% <ø> (ø)
src/daft-core/src/python/series.rs 94.70% <100.00%> (+0.02%) ⬆️
src/daft-core/src/series/array_impl/data_array.rs 95.95% <100.00%> (ø)
...c/daft-core/src/series/array_impl/logical_array.rs 94.44% <100.00%> (ø)
src/daft-core/src/series/ops/list.rs 64.49% <100.00%> (+1.05%) ⬆️
src/daft-local-execution/src/pipeline.rs 95.43% <100.00%> (+0.01%) ⬆️
src/daft-local-execution/src/sinks/sort.rs 96.15% <100.00%> (+0.40%) ⬆️
... and 31 more

... and 28 files with indirect coverage changes

---- 🚨 Try these New Features:

Copy link
Member

@samster25 samster25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!

@universalmind303 universalmind303 merged commit 7922d2d into Eventual-Inc:main Nov 20, 2024
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants