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: add url_download/url_upload as sql function #3575

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frankliee
Copy link

@frankliee frankliee commented Dec 14, 2024

Currently, download_url/upload_url only support DataFrame API, this PR adds download_url/upload_url to sql, which could be used for image processing demo.

df = daft.from_pydict({"urls": [
    "https://user-images.githubusercontent.com/17691182/190476440-28f29e87-8e3b-41c4-9c28-e112e595f558.png",
    "https://user-images.githubusercontent.com/17691182/190476440-28f29e87-8e3b-41c4-9c28-e112e595f558.png",
    "https://user-images.githubusercontent.com/17691182/190476440-28f29e87-8e3b-41c4-9c28-e112e595f558.png",
]})
df = daft.sql("SELECT image_decode(url_download(urls)) FROM df")
df.show()

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

codspeed-hq bot commented Dec 14, 2024

CodSpeed Performance Report

Merging #3575 will improve performances by 48.63%

Comparing frankliee:sql-url (e72676f) with main (e59581c)

Summary

⚡ 2 improvements
✅ 25 untouched benchmarks

Benchmarks breakdown

Benchmark main frankliee:sql-url Change
test_iter_rows_first_row[100 Small Files] 209.1 ms 175.7 ms +19.02%
test_show[100 Small Files] 23.8 ms 16 ms +48.63%

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

Attention: Patch coverage is 54.72973% with 67 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (e59581c) to head (e72676f).

Files with missing lines Patch % Lines
src/daft-sql/src/modules/url.rs 54.42% 67 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3575      +/-   ##
==========================================
- Coverage   77.99%   77.96%   -0.04%     
==========================================
  Files         720      721       +1     
  Lines       88794    88944     +150     
==========================================
+ Hits        69252    69341      +89     
- Misses      19542    19603      +61     
Files with missing lines Coverage Δ
src/daft-functions/src/uri/download.rs 83.46% <ø> (ø)
src/daft-functions/src/uri/mod.rs 100.00% <ø> (ø)
src/daft-functions/src/uri/upload.rs 70.95% <ø> (+0.41%) ⬆️
src/daft-sql/src/functions.rs 81.85% <100.00%> (+0.08%) ⬆️
src/daft-sql/src/modules/url.rs 54.42% <54.42%> (ø)

... and 4 files with indirect coverage changes

@frankliee frankliee closed this Dec 15, 2024
@frankliee frankliee reopened this Dec 15, 2024
@frankliee frankliee changed the title feat: add download_url/upload_url to sql feat: add download_url/upload_url as sql function Dec 15, 2024
@frankliee frankliee force-pushed the sql-url branch 2 times, most recently from cbeb3f9 to 655307a Compare December 15, 2024 05:58
@frankliee frankliee changed the title feat: add download_url/upload_url as sql function feat: add url_download/url_upload as sql function Dec 16, 2024
@jaychia
Copy link
Contributor

jaychia commented Dec 17, 2024

@andrewgazelka could you help get this through?

@frankliee
Copy link
Author

Would you please consider reviewing this PR? @andrewgazelka

@andrewgazelka
Copy link
Member

Would you please consider reviewing this PR? @andrewgazelka

Hey sorry I missed the @ . I will look at now :)

Copy link
Member

@andrewgazelka andrewgazelka left a comment

Choose a reason for hiding this comment

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

I think this PR could benefit from some minor changes

src/daft-sql/src/modules/url.rs Outdated Show resolved Hide resolved
src/daft-sql/src/modules/url.rs Outdated Show resolved Hide resolved
src/daft-sql/src/modules/url.rs Outdated Show resolved Hide resolved
src/daft-sql/src/modules/url.rs Outdated Show resolved Hide resolved
src/daft-sql/src/modules/url.rs Outdated Show resolved Hide resolved
@andrewgazelka
Copy link
Member

LGTM CC @universalmind303 for sanity check as he has touched a lot of code in this area

@andrewgazelka andrewgazelka self-requested a review December 29, 2024 04:10
Copy link
Member

@andrewgazelka andrewgazelka left a comment

Choose a reason for hiding this comment

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

^

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.

3 participants