[I] On the API of NamespaceIdent [iceberg-rust]

["koenichiwa (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org.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