feat(python): add capability to read unity catalog (uc://) uris #3113

omkar-foss · 2025-01-10T06:58:29Z

Description

This adds capability to read directly from uc:// uris using the local catalog-unity crate. This also exposes the UC temporary credentials in storage_options of the DeltaTable instance so polars or similar readers can use it.

Related Issue(s)

closes Add Python-side support for Unity Catalog #3046

Documentation

N/A

github-actions · 2025-01-10T06:58:49Z

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.

crates/catalog-unity/src/models.rs

python/src/lib.rs

omkar-foss · 2025-01-10T07:30:10Z

crates/catalog-unity/src/lib.rs

+    /// Allow http url (e.g. http://localhost:8080/api/2.1/...)
+    /// Supported keys:
+    /// - `unity_allow_http_url`
+    AllowHttpUrl,


This allows users to work with a local (non-https) Unity Catalog REST API with delta-rs.

python/src/lib.rs

omkar-foss · 2025-02-03T14:29:58Z

@ion-elgreco I've updated this PR to include the temp credentials functionality from PR #3078. Cheers.

codecov · 2025-02-03T18:09:27Z

Codecov Report

Attention: Patch coverage is 16.32653% with 82 lines in your changes missing coverage. Please review.

Project coverage is 72.00%. Comparing base (b3efdfc) to head (31f7167).

Files with missing lines	Patch %	Lines
crates/catalog-unity/src/lib.rs	13.82%	80 Missing and 1 partial ⚠️
python/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3113      +/-   ##
==========================================
- Coverage   72.10%   72.00%   -0.10%     
==========================================
  Files         138      138              
  Lines       45320    45414      +94     
  Branches    45320    45414      +94     
==========================================
+ Hits        32678    32701      +23     
- Misses      10567    10635      +68     
- Partials     2075     2078       +3

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

python/src/lib.rs

ion-elgreco · 2025-02-11T20:19:07Z

@omkar-foss looks good from my side!

@hntd187 can you please also take a look. Specifically on whether the credentials need to be refreshed as part of the objectstore? Does UC also support writing temp credentials?

omkar-foss · 2025-02-11T20:23:37Z

I'll need to add a few python tests for this flow, will add tomorrow and then if all good, I suppose we can merge it.

Also please note, the current UC REST integration that we have is more fine-tuned for Databricks Unity Catalog. The Unity Catalog OSS has slightly different response payloads for some APIs. e.g. Generate Temp Credentials API returns r2_temp_credentials and url in the JSON response for Databricks UC but not for OSS UC. So this causes json decoding errors if you try out the current UC client implementation with an OSS UC instance.

I can look into supporting OSS UC smoothly in a separate PR after this one. Cheers.

ion-elgreco · 2025-02-12T07:55:17Z

@omkar-foss we could add an integration test with OSS version to catch these things essier

omkar-foss · 2025-02-12T08:54:15Z

@omkar-foss we could add an integration test with OSS version to catch these things essier

Yes good idea! I'll add a failing test for Temp Creds API with the UC OSS JSON payload along with some other python tests.

This adds capability to read directly from uc:// uris using the local catalog-unity crate. This also exposes the UC temporary credentials in storage_options of the `DeltaTable` instance so polars or similar readers can use it. Signed-off-by: Omkar P <[email protected]>

Signed-off-by: Omkar P <[email protected]>

omkar-foss · 2025-02-12T21:09:21Z

@omkar-foss we could add an integration test with OSS version to catch these things essier

Yes good idea! I'll add a failing test for Temp Creds API with the UC OSS JSON payload along with some other python tests.

Hi, I've added python integration tests for both UC Databricks and UC OSS with their respective mock payloads. Using mockoon as a mock HTTP server in GitHub Actions for the integration tests.

ion-elgreco · 2025-02-13T10:30:36Z

@omkar-foss we could add an integration test with OSS version to catch these things essier

Yes good idea! I'll add a failing test for Temp Creds API with the UC OSS JSON payload along with some other python tests.

Hi, I've added python integration tests for both UC Databricks and UC OSS with their respective mock payloads. Using mockoon as a mock HTTP server in GitHub Actions for the integration tests.

Why not with the actual UC server?

omkar-foss · 2025-02-13T12:16:08Z

Why not with the actual UC server?

Since I had to anyway add a mock server for the UC Databricks integration test, thought might as well use it for UC OSS test to keep it simple, since we're testing only 2 UC REST APIs (Temp Credentials and Get Table Details).

Let me know if you foresee a strong use case for adding the actual UC OSS server, if so, I'll add it here :)

Signed-off-by: Omkar P <[email protected]>

ion-elgreco · 2025-02-13T15:12:51Z

Let's give @hntd187 some time to give a final look, otherwise we can merge end of the week

hntd187

Mostly good, I'd like this to not panic, instead returning errors

hntd187 · 2025-02-13T18:42:14Z

crates/catalog-unity/src/lib.rs

+                temp_creds.get_credentials().unwrap()
+            }
+            TableTempCredentialsResponse::Error(_error) => {
+                panic!("Unable to get temporary credentials from Unity Catalog.")


I don't think being unable to get temp creds should result in a panic, perhaps a nice error result?

hntd187 · 2025-02-13T18:42:54Z

crates/catalog-unity/src/lib.rs

+    ) -> Result<(String, HashMap<String, String>), UnityCatalogError> {
+        let uri_parts: Vec<&str> = table_uri[5..].split('.').collect();
+        if uri_parts.len() != 3 {
+            panic!("Invalid Unity Catalog URI: {}", table_uri);


Return an error instead of panic

hntd187 · 2025-02-13T18:43:15Z

crates/catalog-unity/src/lib.rs

+        let database_name = uri_parts[1];
+        let table_name = uri_parts[2];
+
+        let unity_catalog = match UnityCatalogBuilder::from_env().build() {


just use ? operator instead

hntd187 · 2025-02-13T18:43:26Z

crates/catalog-unity/src/lib.rs

+            .get_table_storage_location(Some(catalog_id.to_string()), database_name, table_name)
+            .await
+        {
+            Ok(s) => s,


just use ?

hntd187 · 2025-02-13T18:45:48Z

crates/catalog-unity/src/lib.rs

+        ));
+
+        let (table_path, temp_creds) = match result {
+            Ok(tup) => tup,


hntd187 · 2025-02-13T18:46:16Z

crates/catalog-unity/src/lib.rs

+
+        let mut storage_options = options.0.clone();
+
+        if !temp_creds.is_empty() {


Does it matter if it's empty? extend should work on an empty collection

Yes, silly me, thanks for this. Done.

Signed-off-by: Omkar P <[email protected]>

omkar-foss · 2025-02-14T06:05:38Z

Mostly good, I'd like this to not panic, instead returning errors

Thanks for your review, Steve. I've updated the PR as per your comments.

Signed-off-by: Omkar P <[email protected]>

ion-elgreco · 2025-02-14T16:34:13Z

@omkar-foss can you add an integration page to the docs? :)

hntd187 · 2025-02-14T19:27:09Z

crates/catalog-unity/src/lib.rs

+            .await?;
+        let credentials = match temp_creds_res {
+            TableTempCredentialsResponse::Success(temp_creds) => {
+                temp_creds.get_credentials().unwrap()


missed an error case here

hntd187

Looks good, just one more error case nit :)

github-actions bot added binding/python Issues for the Python package binding/rust Issues for the Rust crate labels Jan 10, 2025

omkar-foss force-pushed the feat-uc-python branch 2 times, most recently from 8fcd7f4 to 0824abe Compare January 10, 2025 07:03

omkar-foss changed the title ~~feat(python): Add capability to read Unity Catalog (uc://) uris~~ feat(python): add capability to read unity catalog (uc://) uris Jan 10, 2025

omkar-foss commented Jan 10, 2025

View reviewed changes

crates/catalog-unity/src/models.rs Outdated Show resolved Hide resolved

omkar-foss commented Jan 10, 2025

View reviewed changes

python/src/lib.rs Outdated Show resolved Hide resolved

omkar-foss commented Jan 10, 2025

View reviewed changes

ion-elgreco reviewed Jan 10, 2025

View reviewed changes

python/src/lib.rs Outdated Show resolved Hide resolved

omkar-foss force-pushed the feat-uc-python branch from 0da4600 to faf9ba9 Compare January 13, 2025 07:29

rtyler added this to the v0.24 milestone Jan 15, 2025

omkar-foss mentioned this pull request Jan 21, 2025

feat: support reading tables via Unity Catalog provided credentials #3078

Merged

omkar-foss force-pushed the feat-uc-python branch from faf9ba9 to 1a810d9 Compare February 3, 2025 14:23

omkar-foss requested a review from ion-elgreco February 3, 2025 14:27

omkar-foss marked this pull request as ready for review February 3, 2025 14:30

omkar-foss requested review from wjones127, fvaleye, roeap, rtyler and hntd187 as code owners February 3, 2025 14:30

ion-elgreco reviewed Feb 4, 2025

View reviewed changes

python/src/lib.rs Outdated Show resolved Hide resolved

omkar-foss force-pushed the feat-uc-python branch from 1a810d9 to 2fdb6e2 Compare February 11, 2025 20:00

omkar-foss requested a review from ion-elgreco February 11, 2025 20:10

omkar-foss closed this Feb 11, 2025

omkar-foss reopened this Feb 11, 2025

omkar-foss added 2 commits February 13, 2025 02:32

test(python): add tests with mock APIs for Unity Catalog variants

a904e9a

Signed-off-by: Omkar P <[email protected]>

omkar-foss force-pushed the feat-uc-python branch from 2fdb6e2 to a904e9a Compare February 12, 2025 21:03

fix: update mockoon datafiles path for github actions

734d4cb

Signed-off-by: Omkar P <[email protected]>

omkar-foss force-pushed the feat-uc-python branch from 6e482cc to 734d4cb Compare February 13, 2025 12:24

hntd187 requested changes Feb 13, 2025

View reviewed changes

fix: replace panics with proper errors

452c0ab

Signed-off-by: Omkar P <[email protected]>

omkar-foss requested a review from hntd187 February 14, 2025 06:08

chore: reorganize imports

31f7167

Signed-off-by: Omkar P <[email protected]>

hntd187 reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(python): add capability to read unity catalog (uc://) uris #3113

feat(python): add capability to read unity catalog (uc://) uris #3113

omkar-foss commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

omkar-foss Jan 10, 2025

omkar-foss commented Feb 3, 2025

codecov bot commented Feb 3, 2025 •

edited

Loading

ion-elgreco commented Feb 11, 2025 •

edited

Loading

omkar-foss commented Feb 11, 2025 •

edited

Loading

ion-elgreco commented Feb 12, 2025

omkar-foss commented Feb 12, 2025

omkar-foss commented Feb 12, 2025

ion-elgreco commented Feb 13, 2025

omkar-foss commented Feb 13, 2025

ion-elgreco commented Feb 13, 2025

hntd187 left a comment

hntd187 Feb 13, 2025

omkar-foss Feb 14, 2025

hntd187 Feb 13, 2025

omkar-foss Feb 14, 2025

hntd187 Feb 13, 2025

omkar-foss Feb 14, 2025

hntd187 Feb 13, 2025

omkar-foss Feb 14, 2025

hntd187 Feb 13, 2025

omkar-foss Feb 14, 2025

hntd187 Feb 13, 2025

omkar-foss Feb 14, 2025

omkar-foss commented Feb 14, 2025

ion-elgreco commented Feb 14, 2025 •

edited

Loading

hntd187 Feb 14, 2025

hntd187 left a comment


		let mut storage_options = options.0.clone();

		if !temp_creds.is_empty() {

feat(python): add capability to read unity catalog (uc://) uris #3113

Are you sure you want to change the base?

feat(python): add capability to read unity catalog (uc://) uris #3113

Conversation

omkar-foss commented Jan 10, 2025

Description

Related Issue(s)

Documentation

github-actions bot commented Jan 10, 2025

Choose a reason for hiding this comment

omkar-foss commented Feb 3, 2025

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

ion-elgreco commented Feb 11, 2025 • edited Loading

omkar-foss commented Feb 11, 2025 • edited Loading

ion-elgreco commented Feb 12, 2025

omkar-foss commented Feb 12, 2025

omkar-foss commented Feb 12, 2025

ion-elgreco commented Feb 13, 2025

omkar-foss commented Feb 13, 2025

ion-elgreco commented Feb 13, 2025

hntd187 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omkar-foss commented Feb 14, 2025

ion-elgreco commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

hntd187 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 3, 2025 •

edited

Loading

ion-elgreco commented Feb 11, 2025 •

edited

Loading

omkar-foss commented Feb 11, 2025 •

edited

Loading

ion-elgreco commented Feb 14, 2025 •

edited

Loading