On the API of NamespaceIdent
- Dominant language
- Rust
- Stars
- 1.4k
- Forks
- 567
- Avg merge
- 2d 2h
- Merged PRs (30d)
- 93
Description
### 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`. NamespaceIdent is not a smart pointer to [String]. 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]`
2. Remove the `Deref` 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 for NamespaceIdent {
fn as_ref(&self) -> &NamespaceIdentRef {
NamespaceIdentRef::from_parts(&self.0)
}
}
impl Borrow 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 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` 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
Contributor guide
Research direction
Start by locating NamespaceIdent and its current inner, Deref, and parent APIs, then inspect whether the same concerns apply to TableIdent. Compare the proposed owned/borrowed identifier pattern with the existing API and identify the affected callers. Done means the community agrees on the breaking API shape and its scope before implementation begins.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100