feat: move unity catalog integration to it's own crate #3044

hntd187 · 2024-12-02T15:23:32Z

Description

This moves the catalog integration of unity catalog to it's own crate. It also replaces the mock server with the httpmock crate and the custom retry logic with request_middleware's retry middleware. I think this will be easier to manage as a client and I do not believe many people were using this integration anyways.

Related Issue(s)

Documentation

Signed-off-by: Stephen Carman <[email protected]>

…e code base Signed-off-by: Stephen Carman <[email protected]>

Signed-off-by: Stephen Carman <[email protected]>

…024 changes Signed-off-by: Stephen Carman <[email protected]>

Signed-off-by: Stephen Carman <[email protected]>

codecov · 2024-12-02T15:37:15Z

Codecov Report

Attention: Patch coverage is 55.17241% with 104 lines in your changes missing coverage. Please review.

Project coverage is 72.40%. Comparing base (e665ca8) to head (54d777c).

Files with missing lines	Patch %	Lines
crates/catalog-unity/src/client/retry.rs	14.89%	40 Missing ⚠️
crates/catalog-unity/src/lib.rs	52.72%	17 Missing and 9 partials ⚠️
crates/catalog-unity/src/credential.rs	72.54%	5 Missing and 9 partials ⚠️
crates/catalog-unity/src/client/mod.rs	64.70%	4 Missing and 2 partials ⚠️
crates/core/src/writer/stats.rs	54.54%	5 Missing ⚠️
crates/catalog-unity/src/datafusion.rs	0.00%	3 Missing ⚠️
crates/core/src/delta_datafusion/expr.rs	0.00%	3 Missing ⚠️
crates/core/src/kernel/models/actions.rs	50.00%	3 Missing ⚠️
crates/core/src/kernel/snapshot/replay.rs	0.00%	0 Missing and 2 partials ⚠️
crates/core/src/kernel/snapshot/mod.rs	66.66%	0 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3044      +/-   ##
==========================================
- Coverage   72.62%   72.40%   -0.23%     
==========================================
  Files         129      128       -1     
  Lines       41626    41368     -258     
  Branches    41626    41368     -258     
==========================================
- Hits        30232    29951     -281     
- Misses       9419     9445      +26     
+ Partials     1975     1972       -3

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

rtyler · 2024-12-06T03:15:47Z

crates/catalog-unity/src/client/retry.rs

+// impl From<RetryError> for reqwest::Error {
+//     fn from(value: RetryError) -> Self {
+//         Into::into(value)
+//     }
+// }


crates/catalog-unity/src/credential.rs

rtyler · 2024-12-06T03:16:45Z

crates/catalog-unity/src/credential.rs

-                r#"
+        let client = reqwest_middleware::ClientBuilder::new(Client::new()).build();
+
+        let _retry_config = RetryConfig::default();


What's this for?

I wasn't sure if I was going to remove it at that point from the testing, so it's a side effect left over from that.

rtyler · 2024-12-06T03:17:00Z

crates/catalog-unity/src/credential.rs

-            Response::new(Body::from(
-                r#"
+        let client = reqwest_middleware::ClientBuilder::new(Client::new()).build();
+        let _retry_config = RetryConfig::default();


Another unused variable that should be removed? 🤔

crates/core/Cargo.toml

rtyler · 2024-12-06T03:19:45Z

crates/core/Cargo.toml

@@ -137,4 +138,4 @@ datafusion = [
 datafusion-ext = ["datafusion"]
 json = ["parquet/json"]
 python = ["arrow/pyarrow"]
-unity-experimental = ["reqwest", "hyper"]
+unity-experimental = ["reqwest", "reqwest-middleware"]


Shouldn't this feature come off the core crate now that this code is moved into its own crate?

So this is a discussion point I wanted to talk about. The main trait for the catalog and the errors remained in the core crate. So core can build I assume generic catalog support and catalog crates just depend on core and implement. At first glance I kinda didn't have a cohesive error handling thing here, but I think thinking about it. There should be a generic catalog error in core, but have no conditionals features on it. And individual crates implement a conversion to that error type. Sound good?

Co-authored-by: R. Tyler Croy <[email protected]>

Signed-off-by: Stephen Carman <[email protected]>

ion-elgreco · 2024-12-10T21:08:45Z

crates/core/src/operations/merge/filter.rs

-            let compare_expr = match generalize_filter(
+            let compare_expr = generalize_filter(


Why is this changed?

I fixed a few warnings along the way, this statement could be just written with an ? operator instead.

roeap

Overall looks good, just some minor questions.

More gene4rally speaking though, I think we need to make some more deliberate choices around how we want to integrate with catalogs.

AS I have been out for a while, I am not sure where we are in terms of eagerness to push the next release. I would hope that we can do some more cleaning and most importantly testing before we release get "more serious" with unity ...

w.r.t. testing I hope that we can even do some integration tests using OSS UC. Happy the help with that.

roeap · 2024-12-10T20:58:15Z

crates/catalog-unity/src/lib.rs

@@ -216,7 +226,10 @@ impl FromStr for UnityCatalogConfigKey {
            "workspace_url" | "unity_workspace_url" | "databricks_workspace_url" => {
                Ok(UnityCatalogConfigKey::WorkspaceUrl)
            }
-            _ => Err(UnityCatalogError::UnknownConfigKey(s.into()).into()),
+            _ => Err(DataCatalogError::UnknownConfigKey {
+                catalog: "",


I guess in this case the "catalog" would be "Unity".

Btw... a lot fi the client impl was just stolen from Object store, where this field identifies the specific implementation (azure, gcp, ...). If we have this error live in a sepate create, this dioes not make much sense anymore.

roeap · 2024-12-10T21:00:13Z

crates/catalog-unity/src/lib.rs


-        Ok(resp.json().await?)
+        Ok(resp.json().await.map_err(UnityCatalogError::from)?)


if we have an impl From<...> for the error, why do we need the map? does the function return a different kind of err.

roeap · 2024-12-10T21:09:32Z

crates/gcp/tests/context.rs

-            "GOOGLE_SERVICE_ACCOUNT",
-            account_path.as_path().to_str().unwrap(),
-        );
+        unsafe {


Am I missing something, or did we remove unsafe around set_var further above, specifically in crates/aws/src/storage.rs, and here we are adding it?

I have to add it back to the other one, thanks for pointing it out

hntd187 added 7 commits November 29, 2024 11:33

feat: rust 2024 edition, moved unity to its own crate

167973c

Signed-off-by: Stephen Carman <[email protected]>

Merge remote-tracking branch 'origin/main'

8f52fdc

feat: just kidding on rust 2024, fixed several warnings throughout th…

e85bf1d

…e code base Signed-off-by: Stephen Carman <[email protected]>

fixed a few more warnings

bfd9b07

Signed-off-by: Stephen Carman <[email protected]>

fixed a few more warnings, reverted some unsafe additions for prior 2…

d092ef0

…024 changes Signed-off-by: Stephen Carman <[email protected]>

fixed a few more warnings, reverted some unsafe additions for prior 2…

36b5e4f

…024 changes Signed-off-by: Stephen Carman <[email protected]>

Merge remote-tracking branch 'mine/move-unity' into move-unity

e5d2eba

github-actions bot added binding/python Issues for the Python package binding/rust Issues for the Rust crate labels Dec 2, 2024

hntd187 added 2 commits December 2, 2024 10:25

chore: fmt

011bc63

Signed-off-by: Stephen Carman <[email protected]>

Removed more prior unsafe keyword adds because I cannot read

713c92c

Signed-off-by: Stephen Carman <[email protected]>

omkar-foss mentioned this pull request Dec 3, 2024

Add Python-side support for Unity Catalog #3046

Open

Merge branch 'main' into move-unity

2ae4011

rtyler reviewed Dec 6, 2024

View reviewed changes

hntd187 and others added 4 commits December 7, 2024 10:20

Update crates/catalog-unity/src/credential.rs

6688a5a

Co-authored-by: R. Tyler Croy <[email protected]>

Apply suggestions from code review

501d0dd

Co-authored-by: R. Tyler Croy <[email protected]>

Update errors (they still aren't perfect) and addressed some feedback

4eefc72

Signed-off-by: Stephen Carman <[email protected]>

Merge branch 'main' into move-unity

54d777c

hntd187 marked this pull request as ready for review December 10, 2024 20:44

hntd187 requested review from wjones127, fvaleye, roeap and ion-elgreco as code owners December 10, 2024 20:44

ion-elgreco reviewed Dec 10, 2024

View reviewed changes

roeap reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move unity catalog integration to it's own crate #3044

feat: move unity catalog integration to it's own crate #3044

hntd187 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

rtyler Dec 6, 2024

rtyler Dec 6, 2024

hntd187 Dec 7, 2024

rtyler Dec 6, 2024

rtyler Dec 6, 2024

hntd187 Dec 7, 2024

ion-elgreco Dec 10, 2024

hntd187 Dec 11, 2024

roeap left a comment

roeap Dec 10, 2024

roeap Dec 10, 2024

roeap Dec 10, 2024

hntd187 Dec 11, 2024

		let compare_expr = match generalize_filter(
		let compare_expr = generalize_filter(


		Ok(resp.json().await?)
		Ok(resp.json().await.map_err(UnityCatalogError::from)?)

feat: move unity catalog integration to it's own crate #3044

Are you sure you want to change the base?

feat: move unity catalog integration to it's own crate #3044

Conversation

hntd187 commented Dec 2, 2024

Description

Related Issue(s)

Documentation

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

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

roeap 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

codecov bot commented Dec 2, 2024 •

edited

Loading