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

feat(core): make micropartition streamable over tables #3709

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

universalmind303
Copy link
Contributor

Description

makes Micropartition streamable over Table, and makes Table a bit cheaper to clone by wrapping the columns column in an Arc.

The driving force for these changes is that currently the logic to go from micropartition to table does not work nicely withing a streaming context as the get_tables method returns Arc<Vec<Table>>. So you can't easily chain streaming methods when working with micropartitions and tables.

Copy link

codspeed-hq bot commented Jan 17, 2025

CodSpeed Performance Report

Merging #3709 will degrade performances by 31.92%

Comparing universalmind303:mp-stream (23f0958) with main (6b302af)

Summary

❌ 1 regressions
✅ 26 untouched benchmarks

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

Benchmarks breakdown

Benchmark main universalmind303:mp-stream Change
test_iter_rows_first_row[100 Small Files] 193.5 ms 284.1 ms -31.92%

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 70.58824% with 30 lines in your changes missing coverage. Please review.

Project coverage is 77.74%. Comparing base (6b302af) to head (23f0958).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-micropartition/src/micropartition.rs 66.66% 22 Missing ⚠️
src/daft-table/src/lib.rs 52.94% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3709      +/-   ##
==========================================
- Coverage   77.94%   77.74%   -0.21%     
==========================================
  Files         725      725              
  Lines       90952    91467     +515     
==========================================
+ Hits        70895    71110     +215     
- Misses      20057    20357     +300     
Files with missing lines Coverage Δ
src/daft-connect/src/execute.rs 84.15% <100.00%> (ø)
src/daft-table/src/ops/explode.rs 87.67% <100.00%> (ø)
src/daft-table/src/ops/joins/hash_join.rs 90.82% <100.00%> (+0.05%) ⬆️
src/daft-table/src/ops/joins/mod.rs 91.32% <100.00%> (+0.10%) ⬆️
src/daft-table/src/ops/unpivot.rs 94.00% <100.00%> (+0.52%) ⬆️
src/daft-table/src/lib.rs 84.14% <52.94%> (+0.40%) ⬆️
src/daft-micropartition/src/micropartition.rs 89.28% <66.66%> (-1.64%) ⬇️

... and 15 files with indirect coverage changes

Copy link
Member

@kevinzwang kevinzwang left a comment

Choose a reason for hiding this comment

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

LGTM

@universalmind303 universalmind303 merged commit 03fea9c into Eventual-Inc:main Jan 21, 2025
39 of 41 checks passed
@universalmind303 universalmind303 deleted the mp-stream branch January 23, 2025 06:33
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