Skip to content

Conversation

@fvaleye
Copy link
Collaborator

@fvaleye fvaleye commented Aug 25, 2025

Description

Use the URL crate in Rust for accessing the DeltaTable. Use only the string-based API in Python.

  • Rust APIs now take url::Url for opening/creating tables.
  • Python keeps str at the boundary and parses to URL immediately.
  • String-based Rust helpers still exist and are marked deprecated (for migration).

Related Issue(s)

Documentation

  • Use URL in Rust instead of using &str
  • Keep &str for the Python binding with helper methods
  • Keep backward compatibility by making the URI deprecated.

Side notes: 💚
@rtyler, I started contributing to delta-rs with my first PR regarding URI.
So, as a "comeback", I'm taking this opportunity to improve it 😄

Copilot AI review requested due to automatic review settings August 25, 2025 14:02
@fvaleye fvaleye added binding/python Issues for the Python package binding/rust Issues for the Rust crate labels Aug 25, 2025
@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch from 249d970 to 045bad4 Compare August 25, 2025 14:02
@github-actions
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@codecov
Copy link

codecov bot commented Aug 25, 2025

Codecov Report

❌ Patch coverage is 74.57627% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.36%. Comparing base (437718b) to head (1159c4b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/table/builder.rs 55.81% 11 Missing and 8 partials ⚠️
python/src/lib.rs 0.00% 19 Missing ⚠️
crates/core/src/operations/mod.rs 22.22% 13 Missing and 1 partial ⚠️
crates/test/src/read.rs 0.00% 0 Missing and 10 partials ⚠️
crates/core/src/protocol/checkpoints.rs 45.45% 5 Missing and 1 partial ⚠️
crates/core/src/protocol/mod.rs 71.42% 6 Missing ⚠️
crates/core/src/kernel/snapshot/mod.rs 28.57% 0 Missing and 5 partials ⚠️
crates/core/src/test_utils/mod.rs 28.57% 3 Missing and 2 partials ⚠️
crates/catalog-unity/src/datafusion.rs 0.00% 4 Missing ⚠️
python/src/filesystem.rs 0.00% 4 Missing ⚠️
... and 11 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3707      +/-   ##
==========================================
- Coverage   75.38%   75.36%   -0.02%     
==========================================
  Files         145      145              
  Lines       43690    43955     +265     
  Branches    43690    43955     +265     
==========================================
+ Hits        32935    33127     +192     
- Misses       9161     9222      +61     
- Partials     1594     1606      +12     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch from 045bad4 to 2410c54 Compare August 25, 2025 14:06
@rtyler rtyler self-assigned this Aug 25, 2025
@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch from 2410c54 to 9f19896 Compare August 25, 2025 14:08
@fvaleye fvaleye changed the title feature(url): use Url in Rust for accessing to DeltaTable, use only string-based api in Python feat(url): use Url in Rust for accessing to DeltaTable, use only string-based api in Python Aug 25, 2025
@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch from 0fb5b46 to 8a4c58b Compare August 25, 2025 14:17
@rtyler
Copy link
Member

rtyler commented Aug 25, 2025

@fvaleye This has some conflicts after some concurrent changes were happening by @roeap , I can resolve these conflicts and force push if that's okay with you

@fvaleye
Copy link
Collaborator Author

fvaleye commented Aug 25, 2025

@fvaleye This has some conflicts after some concurrent changes were happening by @roeap , I can resolve these conflicts and force push if that's okay with you

No worries @rtyler, I'm on it 👍

@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch 2 times, most recently from 9e14c5f to 9b56090 Compare August 25, 2025 14:29
roeap
roeap previously approved these changes Aug 25, 2025
Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

That must have been fun tucking about every test that opens a table :).

Thanks for taking care of this.

/// Will fail fast if specified `table_uri` is a local path but doesn't exist.
pub async fn open_table(table_uri: impl AsRef<str>) -> Result<DeltaTable, DeltaTableError> {
let table = DeltaTableBuilder::from_valid_uri(table_uri)?.load().await?;
pub async fn open_table(table_uri: Url) -> Result<DeltaTable, DeltaTableError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thinking out loud .. with object store we do have reqwest in our dependency tree anyways, which exposes an IntoUrl trait ... however then we would have a coupling with the requwest version as we now expose its types, so not worth it.

Copy link
Collaborator Author

@fvaleye fvaleye Aug 25, 2025

Choose a reason for hiding this comment

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

Let me put a nice cargo tree 😄

either → … → reqwest v0.12.15
delta_kernel v0.14.0 → reqwest v0.12.15
deltalake-core v0.28.0 → reqwest v0.12.15
deltalake-lakefs v0.11.0 → reqwest v0.12.15

I'm still searching, but idk where we could use the IntoUrl... should we safely deprecate it?

@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch 2 times, most recently from 40bfca3 to c7c31a5 Compare August 25, 2025 15:19
@fvaleye
Copy link
Collaborator Author

fvaleye commented Aug 25, 2025

That must have been fun tucking about every test that opens a table :).

Fun? I want to know your hobbies @roeap 🤣
It was more of a pain to "play" with the different paths in the codebase, as I started working on a cloning feature...

@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch 5 times, most recently from 8e2d4f5 to 3d17fdd Compare August 25, 2025 15:46
@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch 3 times, most recently from 79e8aad to 95b1fcd Compare September 1, 2025 08:02
@fvaleye
Copy link
Collaborator Author

fvaleye commented Sep 1, 2025

All good @rtyler @roeap 🚀

rtyler
rtyler previously approved these changes Sep 1, 2025
@rtyler rtyler disabled auto-merge September 1, 2025 14:14
@rtyler rtyler enabled auto-merge (rebase) September 1, 2025 14:14
@rtyler rtyler closed this Sep 1, 2025
auto-merge was automatically disabled September 1, 2025 14:14

Pull request was closed

@rtyler rtyler reopened this Sep 1, 2025
@rtyler rtyler enabled auto-merge (rebase) September 1, 2025 14:14
auto-merge was automatically disabled September 1, 2025 14:34

Rebase failed

@rtyler rtyler force-pushed the feature/use-url-in-for-referring-tables branch from 95b1fcd to 72548b0 Compare September 1, 2025 15:00
@rtyler rtyler enabled auto-merge (rebase) September 1, 2025 15:00
@rtyler rtyler force-pushed the feature/use-url-in-for-referring-tables branch from 72548b0 to 5b2a309 Compare September 2, 2025 00:02
@fvaleye fvaleye force-pushed the feature/use-url-in-for-referring-tables branch 3 times, most recently from 4fc2878 to c1260ce Compare September 2, 2025 07:50
rtyler
rtyler previously approved these changes Sep 2, 2025
auto-merge was automatically disabled September 2, 2025 13:27

Rebase failed

@rtyler rtyler force-pushed the feature/use-url-in-for-referring-tables branch from c1260ce to d5195c1 Compare September 2, 2025 13:37
@rtyler rtyler enabled auto-merge (rebase) September 2, 2025 13:37
roeap
roeap previously approved these changes Sep 2, 2025
Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

LGTM!

@rtyler rtyler merged commit b941b6c into delta-io:main Sep 2, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

binding/python Issues for the Python package binding/rust Issues for the Rust crate delta-inspect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt url::Url based interfaces for referring to tables

4 participants