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(expressions): Extend Expression.url.upload() to support row-specific URLs #3518

Merged

Conversation

desmondcheongzx
Copy link
Contributor

Addresses #3320

Expression.url.upload() can now take in a column of urls to upload each row to a specific url.

@github-actions github-actions bot added the feat label Dec 8, 2024
Copy link

codspeed-hq bot commented Dec 8, 2024

CodSpeed Performance Report

Merging #3518 will improve performances by 56.52%

Comparing desmondcheongzx:allow-upload-to-take-col (62817c6) with main (b87e0a3)

Summary

⚡ 1 improvements
✅ 26 untouched benchmarks

Benchmarks breakdown

Benchmark main desmondcheongzx:allow-upload-to-take-col Change
test_iter_rows_first_row[100 Small Files] 257.3 ms 164.4 ms +56.52%

@desmondcheongzx desmondcheongzx force-pushed the allow-upload-to-take-col branch from cae7581 to c12cf8e Compare December 8, 2024 06:54
@desmondcheongzx desmondcheongzx force-pushed the allow-upload-to-take-col branch from c12cf8e to 5badcb4 Compare December 8, 2024 07:00
Copy link

codecov bot commented Dec 8, 2024

Codecov Report

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

Project coverage is 77.91%. Comparing base (8ffc4ff) to head (62817c6).
Report is 54 commits behind head on main.

Files with missing lines Patch % Lines
src/daft-functions/src/uri/upload.rs 76.92% 27 Missing ⚠️
src/daft-io/src/lib.rs 71.42% 2 Missing ⚠️
daft/expressions/expressions.py 85.71% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3518      +/-   ##
==========================================
+ Coverage   77.60%   77.91%   +0.31%     
==========================================
  Files         708      719      +11     
  Lines       86156    88516    +2360     
==========================================
+ Hits        66858    68966    +2108     
- Misses      19298    19550     +252     
Files with missing lines Coverage Δ
src/daft-functions/src/python/uri.rs 87.75% <100.00%> (+1.08%) ⬆️
src/daft-functions/src/uri/mod.rs 100.00% <100.00%> (ø)
daft/expressions/expressions.py 93.51% <85.71%> (-0.08%) ⬇️
src/daft-io/src/lib.rs 75.71% <71.42%> (+1.80%) ⬆️
src/daft-functions/src/uri/upload.rs 70.53% <76.92%> (+5.90%) ⬆️

... and 118 files with indirect coverage changes

@ccmao1130 ccmao1130 added the p1 Important to tackle soon, but preemptable by p0 label Dec 17, 2024
@desmondcheongzx desmondcheongzx requested review from jaychia and raunakab and removed request for jaychia December 18, 2024 01:43
@desmondcheongzx
Copy link
Contributor Author

@raunakab assigning you for review since you're oncall :)

@raunakab
Copy link
Contributor

Perfect! Will take a look through.

Copy link
Contributor

@raunakab raunakab left a comment

Choose a reason for hiding this comment

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

Just a few questions.

src/daft-functions/src/uri/upload.rs Show resolved Hide resolved
src/daft-functions/src/uri/upload.rs Outdated Show resolved Hide resolved
@desmondcheongzx desmondcheongzx merged commit 1a4ae66 into Eventual-Inc:main Dec 21, 2024
41 checks passed
@desmondcheongzx desmondcheongzx deleted the allow-upload-to-take-col branch December 21, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat p1 Important to tackle soon, but preemptable by p0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants