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.

Sign up for GitHub

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

refactor: rm async_trait and add trait_variant #186

Closed
wants to merge 1 commit into from
Closed

refactor: rm async_trait and add trait_variant #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@

[workspace]
resolver = "2"
members = ["crates/catalog/*", "crates/examples", "crates/iceberg", "crates/test_utils"]
members = [
"crates/catalog/*",
"crates/examples",
"crates/iceberg",
"crates/test_utils",
]

[workspace.package]
version = "0.1.0"
Expand All @@ -33,7 +38,6 @@ apache-avro = "0.16"
arrow-arith = { version = ">=46" }
arrow-array = { version = ">=46" }
arrow-schema = { version = ">=46" }
async-trait = "0.1"
bimap = "0.6"
bitvec = "1.0.1"
chrono = "0.4"
Expand Down
1 change: 0 additions & 1 deletion crates/catalog/hms/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ license = { workspace = true }
keywords = ["iceberg", "hive", "catalog"]

[dependencies]
async-trait = { workspace = true }
hive_metastore = "0.0.1"
iceberg = { workspace = true }
# the thrift upstream suffered from no regular rust release.
Expand Down
2 changes: 0 additions & 2 deletions crates/catalog/hms/src/catalog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
// under the License.

use super::utils::*;
use async_trait::async_trait;
use hive_metastore::{TThriftHiveMetastoreSyncClient, ThriftHiveMetastoreSyncClient};
use iceberg::table::Table;
use iceberg::{Catalog, Namespace, NamespaceIdent, Result, TableCommit, TableCreation, TableIdent};
Expand Down Expand Up @@ -89,7 +88,6 @@ impl HmsCatalog {
}

/// Refer to <https://github.com/apache/iceberg/blob/main/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java> for implementation details.
#[async_trait]
impl Catalog for HmsCatalog {
/// HMS doesn't support nested namespaces.
///
Expand Down
2 changes: 0 additions & 2 deletions crates/catalog/rest/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ license = { workspace = true }
keywords = ["iceberg", "rest", "catalog"]

[dependencies]
# async-trait = { workspace = true }
async-trait = { workspace = true }
chrono = { workspace = true }
iceberg = { workspace = true }
log = "0.4.20"
Expand Down
4 changes: 1 addition & 3 deletions crates/catalog/rest/src/catalog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

use std::collections::HashMap;

use async_trait::async_trait;
use reqwest::header::{self, HeaderMap, HeaderName, HeaderValue};
use reqwest::{Client, Request, Response, StatusCode};
use serde::de::DeserializeOwned;
Expand Down Expand Up @@ -215,7 +214,6 @@ pub struct RestCatalog {
client: HttpClient,
}

#[async_trait]
impl Catalog for RestCatalog {
/// List namespaces from table.
async fn list_namespaces(
Expand Down Expand Up @@ -1558,7 +1556,7 @@ mod tests {
"type": "NoSuchTableException",
"code": 404
}
}
}
"#,
)
.create_async()
Expand Down
2 changes: 1 addition & 1 deletion crates/iceberg/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ apache-avro = { workspace = true }
arrow-arith = { workspace = true }
arrow-array = { workspace = true }
arrow-schema = { workspace = true }
async-trait = { workspace = true }
bimap = { workspace = true }
bitvec = { workspace = true }
chrono = { workspace = true }
Expand All @@ -55,6 +54,7 @@ serde_derive = { workspace = true }
serde_json = { workspace = true }
serde_repr = { workspace = true }
serde_with = { workspace = true }
trait-variant = "0.1.1"
typed-builder = { workspace = true }
url = { workspace = true }
urlencoding = { workspace = true }
Expand Down
20 changes: 10 additions & 10 deletions crates/iceberg/src/catalog/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@ use crate::spec::{
};
use crate::table::Table;
use crate::{Error, ErrorKind, Result};
use async_trait::async_trait;
use std::collections::HashMap;
use std::mem::take;
use std::ops::Deref;
use typed_builder::TypedBuilder;
use uuid::Uuid;

/// The catalog API for Iceberg Rust.
#[async_trait]
pub trait Catalog: std::fmt::Debug {
/// see https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html
#[trait_variant::make(Catalog: Send)]
pub trait LocalCatalog: std::fmt::Debug {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this LocalCatalog variant? I think it would be enough to have a thread safe one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the better approach would be like FileWriter trait, which adds Send for each return type:

pub trait Catalog: Debug + Send {
  fn list_namespace(&self, parent: Option<&NamespeceIdent>) -> impl Future<Output = 
Result<Vec<NamespaceIdent>>> + Send;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal was to minimize code changes as much as possible. There are 13 methods and 2 implementations of Catalog, while trait_variant is a one-line change. I personally prefer async fn over impl Future since it looks neater. But it is true that LocalCatalog is never used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitating adding this LocalCatalog trait. Also async fn seems much more concise to me. Maybe we should close this pr before finding a better approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rust-lang/impl-trait-utils#27

They are adding a rewrite feature to rewrite a trait instead of creating 2 variants. We can wait till it's done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's great! Let's wait for this.

/// List namespaces from table.
async fn list_namespaces(&self, parent: Option<&NamespaceIdent>)
-> Result<Vec<NamespaceIdent>>;
Expand Down Expand Up @@ -505,7 +505,7 @@ mod tests {
{
"type": "assert-ref-snapshot-id",
"ref": "snapshot-name",
"snapshot-id": null
"snapshot-id": null
}
"#,
TableRequirement::RefSnapshotIdMatch {
Expand Down Expand Up @@ -625,7 +625,7 @@ mod tests {
{
"action": "assign-uuid",
"uuid": "2cc52516-5e73-41f2-b139-545d41a4e151"
}
}
"#,
TableUpdate::AssignUuid {
uuid: uuid!("2cc52516-5e73-41f2-b139-545d41a4e151"),
Expand All @@ -640,7 +640,7 @@ mod tests {
{
"action": "upgrade-format-version",
"format-version": 2
}
}
"#,
TableUpdate::UpgradeFormatVersion {
format_version: FormatVersion::V2,
Expand Down Expand Up @@ -932,7 +932,7 @@ mod tests {
1,
2
]
}
}
"#;

let update = TableUpdate::RemoveSnapshots {
Expand Down Expand Up @@ -990,7 +990,7 @@ mod tests {
"min-snapshots-to-keep": 2,
"max-snapshot-age-ms": 3,
"max-ref-age-ms": 4
}
}
"#;

let update = TableUpdate::SetSnapshotRef {
Expand All @@ -1017,7 +1017,7 @@ mod tests {
"prop1": "v1",
"prop2": "v2"
}
}
}
"#;

let update = TableUpdate::SetProperties {
Expand All @@ -1041,7 +1041,7 @@ mod tests {
"prop1",
"prop2"
]
}
}
"#;

let update = TableUpdate::RemoveProperties {
Expand Down
Loading