koenichiwa opened a new issue, #2472:
URL: https://github.com/apache/iceberg-rust/issues/2472
### Is your feature request related to a problem or challenge?
While I was working on a TUI application that depended on iceberg-rust, I
noticed a couple of things in the `NamespaceIdent` API that surprised me. They
are somewhat related, so I'll combine them in this issue. I can split the
issues up if requested.
First I needed to have the string representation of each of the parts, and I
instinctively tried to do
```rust
let parts: &[String] = namespace_ident.inner();
```
But surprisingly `inner` consumes the `NamespaceIdent`. Normally in the Rust
APIs I encountered, this is done via `into_inner`, while `inner` only takes a
reference.
Then I noticed that `NamespaceIdent` implements `Deref<Target=[String]>`.
NamespaceIdent is not a smart pointer to [String] and it leaks implementation
details and breaks with Rust's idea of "explicit is better than implicit". It
also causes the following code to run without issue, which is IMHO unintuitive.
```rust
let namespace_ident = iceberg::NamespaceIdent::new("".to_string());
assert!(!namespace_ident.is_empty());
assert_eq!(namespace_ident.len(), 1);
```
Lastly I noticed that functions like `NamespaceIdent.parent` actually create
a new copy of the NamespaceIdent, where I expected to get some sort of
slice/reference to the parent namespace, because that information is already in
the struct itself. There is currently no way to represent a "view" of a
NamespaceIdent
(Some of these points could apply to `TableIdent` as well.)
### Describe the solution you'd like
I want to propose the following changes:
1. Rename `inner` to `into_inner`. Create a new `inner` function that
returns `&[String]` (or even implement `AsSlice`)
2. Remove the `Deref<Target=[String]>` implementation from NamespaceIdent.
Instead the new `inner` function can be used when users want to explicitly
iterate or index into the separate parts of the identifier.
3. NamespaceIdent is inherently a path into a catalog. We can introduce a
new struct that aligns NamespaceIdent with the idiomatic Path/PathBuf or
str/String pattern found in the standard library, where the owned type
dereferences to a custom borrowed type, i.e. the "view".
I have a rough sketch below:
```rust
#[repr(transparent)]
pub struct NamespaceIdentRef([String]);
impl NamespaceIdentRef {
fn from_parts(parts: &[String]) -> &Self {
// SAFETY: `NamespaceIdentRef` is a `#[repr(transparent)]` wrapper
around `[String]`.
// Therefore, it is safe to cast `&[String]` to `&NamespaceIdentRef`.
unsafe { &*(std::ptr::from_ref(parts) as *const Self) }
}
}
impl AsRef<NamespaceIdentRef> for NamespaceIdent {
fn as_ref(&self) -> &NamespaceIdentRef {
NamespaceIdentRef::from_parts(&self.0)
}
}
impl Borrow<NamespaceIdentRef> for NamespaceIdent {
fn borrow(&self) -> &NamespaceIdentRef {
self.as_ref()
}
}
impl ToOwned for NamespaceIdentRef {
type Owned = NamespaceIdent;
fn to_owned(&self) -> Self::Owned {
NamespaceIdent(self.0.to_vec())
}
}
```
We can add in `Deref<Target=NamespaceIdentRef> for NamespaceIdent` without
leaking any implementation details. This will also allow several functions to
actually return without copying any data. For instance we can move `parent`:
```rust
impl NamespaceIdentRef {
pub fn parent(&self) -> Option<&Self> {
self.0.split_last().and_then(|(_, parent)| {
if parent.is_empty() {
None
} else {
Some(Self::from_parts(parent))
}
})
}
}
```
NB: NamespaceIdentRef will not implement `Deref<Target=[String]>` because
that will leak implementation details again.
I am aware that all of these are breaking changes, however I believe that
this can improve this part of the API, and might even prevent breaking changes
in the future.
I'm willing to provide one or more PRs, depending on how this issue needs to
be split up. I've actually already done most of the implementation as part of
my research.
### Willingness to contribute
I would be willing to contribute to this feature with guidance from the
Iceberg Rust community
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
