feat(Rust): add DataType support #822
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #822 +/- ##
============================================
+ Coverage 77.18% 77.59% +0.40%
Complexity 607 607
============================================
Files 85 86 +1
Lines 8858 9020 +162
Branches 1043 1043
============================================
+ Hits 6837 6999 +162
Misses 1781 1781
Partials 240 240 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds Rust bindings for GraphAr's DataType API via FFI, enabling type-safe representation of GraphAr data types in Rust. Key additions include FFI bindings, safe Rust wrappers, comprehensive tests, and build configuration updates.
- Implements
cxx-based FFI bindings for C++DataTypeandTypeenum - Provides safe Rust wrapper with constructors, trait implementations, and accessor methods
- Adds comprehensive unit tests covering equality, formatting, nested types, and type introspection
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/types.rs | New module implementing DataType wrapper with PartialEq, Display, Debug traits and comprehensive tests |
| rust/src/lib.rs | Exports new types module as public API |
| rust/src/ffi.rs | Defines FFI bridge with Type enum and DataType C++ bindings |
| rust/src/graphar_rs.cc | Implements C++ shim function to_type_name for type formatting |
| rust/include/graphar_rs.h | Declares C++ shim function and includes necessary headers |
| rust/build.rs | Updates CMake configuration to disable sanitizer and set static build flag |
| .github/workflows/rust.yaml | Adds release build and test steps to CI workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/src/types.rs
Outdated
| if self.0.is_null() { | ||
| write!(f, "null") | ||
| } else { | ||
| write!( | ||
| f, | ||
| "{}", | ||
| ffi::graphar::to_type_name(self.0.as_ref().unwrap()) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The Display and Debug implementations are identical. This code duplication makes maintenance harder. Consider implementing Debug to delegate to Display, or extract the common logic into a helper method.
| if self.0.is_null() { | |
| write!(f, "null") | |
| } else { | |
| write!( | |
| f, | |
| "{}", | |
| ffi::graphar::to_type_name(self.0.as_ref().unwrap()) | |
| ) | |
| } | |
| Display::fmt(self, f) |
| #[derive(Clone)] | ||
| pub struct DataType(SharedPtr<ffi::graphar::DataType>); | ||
|
|
||
| impl PartialEq for DataType { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| if self.0.is_null() && other.0.is_null() { | ||
| return true; | ||
| } | ||
| if self.0.is_null() || other.0.is_null() { | ||
| return false; | ||
| } | ||
|
|
||
| self.0.Equals(other.0.as_ref().expect("rhs is nullptr")) | ||
| } | ||
| } | ||
|
|
||
| impl Eq for DataType {} | ||
|
|
||
| impl Display for DataType { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| if self.0.is_null() { | ||
| write!(f, "null") | ||
| } else { | ||
| write!( | ||
| f, | ||
| "{}", | ||
| ffi::graphar::to_type_name(self.0.as_ref().unwrap()) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Debug for DataType { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| if self.0.is_null() { | ||
| write!(f, "null") | ||
| } else { | ||
| write!( | ||
| f, | ||
| "{}", | ||
| ffi::graphar::to_type_name(self.0.as_ref().unwrap()) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl DataType { | ||
| pub fn value_type(&self) -> Self { | ||
| DataType(self.0.value_type().clone()) | ||
| } | ||
|
|
||
| pub fn id(&self) -> Type { | ||
| self.0.id() | ||
| } | ||
|
|
||
| pub fn bool() -> Self { | ||
| Self(ffi::graphar::boolean().clone()) | ||
| } | ||
|
|
||
| pub fn int32() -> Self { | ||
| Self(ffi::graphar::int32().clone()) | ||
| } | ||
|
|
||
| pub fn int64() -> Self { | ||
| Self(ffi::graphar::int64().clone()) | ||
| } | ||
|
|
||
| pub fn float32() -> Self { | ||
| Self(ffi::graphar::float32().clone()) | ||
| } | ||
|
|
||
| pub fn float64() -> Self { | ||
| Self(ffi::graphar::float64().clone()) | ||
| } | ||
|
|
||
| pub fn string() -> Self { | ||
| Self(ffi::graphar::string().clone()) | ||
| } | ||
|
|
||
| pub fn date() -> Self { | ||
| Self(ffi::graphar::date().clone()) | ||
| } | ||
|
|
||
| pub fn timestamp() -> Self { | ||
| Self(ffi::graphar::timestamp().clone()) | ||
| } | ||
|
|
||
| pub fn list(value_type: &DataType) -> Self { | ||
| Self(ffi::graphar::list(&value_type.0)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The public struct DataType and its public methods lack documentation. Consider adding doc comments to explain what DataType represents, its purpose, and provide usage examples. This is especially important for public APIs that users will interact with directly.
| pub fn value_type(&self) -> Self { | ||
| DataType(self.0.value_type().clone()) | ||
| } | ||
|
|
||
| pub fn id(&self) -> Type { |
There was a problem hiding this comment.
The public methods value_type() and id() don't check if the underlying SharedPtr is null before dereferencing. This could lead to undefined behavior or panics if called on a DataType with a null pointer. Consider adding null checks or documenting that these methods should not be called on null DataTypes.
| pub fn value_type(&self) -> Self { | |
| DataType(self.0.value_type().clone()) | |
| } | |
| pub fn id(&self) -> Type { | |
| pub fn value_type(&self) -> Self { | |
| if self.0.is_null() { | |
| panic!("Called DataType::value_type() on a null DataType"); | |
| } | |
| DataType(self.0.value_type().clone()) | |
| } | |
| pub fn id(&self) -> Type { | |
| if self.0.is_null() { | |
| panic!("Called DataType::id() on a null DataType"); | |
| } |
There was a problem hiding this comment.
SharedPtr<T> implement Deref<Target = T>, and if the ptr is null, deref will panic.
see https://docs.rs/cxx/latest/src/cxx/shared_ptr.rs.html#260-275
self.0 is SharedPtr<ffi::graphar::DataType>
self.0.value_type() is actually Deref::deref(&self.0).value_type().
So do we need to double check if this ptr is null?
| Self(ffi::graphar::timestamp().clone()) | ||
| } | ||
|
|
||
| pub fn list(value_type: &DataType) -> Self { |
There was a problem hiding this comment.
The list() method doesn't validate if value_type has a null pointer before passing it to the FFI function. Consider adding a null check or documenting this precondition to prevent potential undefined behavior.
| pub fn list(value_type: &DataType) -> Self { | |
| pub fn list(value_type: &DataType) -> Self { | |
| if value_type.0.is_null() { | |
| panic!("DataType::list called with null value_type"); | |
| } |
There was a problem hiding this comment.
Actually, when the inner type is null, this function won't panic. Only when we try to deref this pointer, it will panic.
see https://docs.rs/cxx/latest/src/cxx/shared_ptr.rs.html#260-275
rust/src/types.rs
Outdated
| return false; | ||
| } | ||
|
|
||
| self.0.Equals(other.0.as_ref().expect("rhs is nullptr")) |
There was a problem hiding this comment.
The error message "rhs is nullptr" is inconsistent with Rust terminology. In Rust, the equivalent concept is called "null pointer" or simply "null", not "nullptr" (which is C++ terminology). Additionally, this expect call is unreachable because the condition on line 33 already ensures that other.0 is not null at this point. Consider removing the expect call and using unwrap() instead, or better yet, use as_ref().unwrap() which is more idiomatic.
| self.0.Equals(other.0.as_ref().expect("rhs is nullptr")) | |
| self.0.Equals(other.0.as_ref().unwrap()) |
2 participants
Reason for this PR
add
DataTypefor Rust binding. #821What changes are included in this PR?
DataTypeand corresponding testsGRAPHAR_ENABLE_SANITIZER=OFFfor avoiding linking address sanitizer in RustAre these changes tested?
Yes
Are there any user-facing changes?
Yes
Summary
cxx-based Rust bindings for GraphArType+DataType(FFI insrc/ffi.rs) and wire them into the crate API (src/lib.rs,src/types.rs).graphar_rs::to_type_name(const graphar::DataType&) -> rust::String(include/graphar_rs.h,src/graphar_rs.cc) forDataType::ToTypeName()so Rust can format types.types::DataTypearoundSharedPtr<ffi::graphar::DataType>with constructors,id(),value_type(),list(),PartialEq/Eq, plus null-awareDisplay/Debug.id, andvalue_type(src/types.rs).build.rsto build/link the C++ side (CMake +cxx_build) withGRAPHAR_ENABLE_SANITIZER=OFF.