You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "odysa (via GitHub)" <gi...@apache.org> on 2024/02/03 15:59:09 UTC
[PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
odysa opened a new pull request, #186:
URL: https://github.com/apache/iceberg-rust/pull/186
Close #139
Use `trait_variant::make` to support async fn in pub traits. It creates 2 traits ---- `LocalCatalog` for single thread and `Catalog` with `Send` for multithreaded runtime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#issuecomment-1952283128
cc @odysa Should we close this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#discussion_r1477552350
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -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 {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "odysa (via GitHub)" <gi...@apache.org>.
odysa commented on PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#issuecomment-1952945242
@liurenjie1024 Yes, we can close this and reopen it after they release the rewrite feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#discussion_r1477570301
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -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 {
Review Comment:
Yeah, that's great! Let's wait for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#issuecomment-1953347525
> @liurenjie1024 Yes, we can close this and reopen it after they release the rewrite feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#discussion_r1477195324
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -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 {
Review Comment:
I think the better approach would be like `FileWriter` trait, which adds `Send` for each return type:
```rust
pub trait Catalog: Debug + Send {
fn list_namespace(&self, parent: Option<&NamespeceIdent>) -> impl Future<Output =
Result<Vec<NamespaceIdent>>> + Send;
}
```
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -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 {
Review Comment:
Why we need this `LocalCatalog` variant? I think it would be enough to have a thread safe one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "odysa (via GitHub)" <gi...@apache.org>.
odysa commented on code in PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#discussion_r1477561928
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -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 {
Review Comment:
https://github.com/rust-lang/impl-trait-utils/pull/27
They are adding a `rewrite` feature to rewrite a trait instead of creating 2 variants. We can wait till it's done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "odysa (via GitHub)" <gi...@apache.org>.
odysa commented on code in PR #186:
URL: https://github.com/apache/iceberg-rust/pull/186#discussion_r1477382004
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -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 {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor: rm async_trait and add trait_variant [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 closed pull request #186: refactor: rm async_trait and add trait_variant
URL: https://github.com/apache/iceberg-rust/pull/186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org