Copilot commented on code in PR #830: URL: https://github.com/apache/incubator-graphar/pull/830#discussion_r2703725860
########## rust/src/property.rs: ########## @@ -0,0 +1,432 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Rust bindings for GraphAr property types. + +use std::ops::{Deref, DerefMut}; + +use crate::ffi; +use crate::types::{Cardinality, DataType, FileType}; +use cxx::{CxxVector, SharedPtr, UniquePtr, let_cxx_string}; + +/// A property definition in the GraphAr schema. +/// +/// This is a thin wrapper around the C++ `graphar::Property`. +pub struct Property(pub(crate) UniquePtr<ffi::graphar::Property>); + +impl Property { + /// Create a new property definition. + /// + /// Note: In upstream GraphAr C++ (`graphar::Property` constructor), a primary key property is + /// always treated as non-nullable. Concretely, `is_nullable` is forced to `false` when + /// `is_primary` is `true` (regardless of the provided `is_nullable` value). + /// + /// This method takes `data_type` by value. Clone it if you need to reuse it. + pub fn new<S: AsRef<str>>( + name: S, + data_type: DataType, + is_primary: bool, + is_nullable: bool, + cardinality: Cardinality, + ) -> Self { + let_cxx_string!(name = name.as_ref()); + Self(ffi::graphar::new_property( + &name, + data_type.0, + is_primary, + is_nullable, + cardinality, + )) + } + + /// Return the property name. + pub fn name(&self) -> String { + crate::cxx_string_to_string(ffi::graphar::property_get_name(&self.0)) + } + + /// Return the property data type. + pub fn data_type(&self) -> DataType { + let ty = ffi::graphar::property_get_type(&self.0); + DataType(ty.clone()) + } + + /// Return whether this property is a primary key. + pub fn is_primary(&self) -> bool { + ffi::graphar::property_is_primary(&self.0) + } + + /// Return whether this property is nullable. + /// + /// Note: In upstream GraphAr C++ (`graphar::Property` constructor), a primary key property is + /// always treated as non-nullable, so this returns `false` when `is_primary()` is `true`. + pub fn is_nullable(&self) -> bool { + ffi::graphar::property_is_nullable(&self.0) + } + + /// Return the cardinality of this property. + pub fn cardinality(&self) -> Cardinality { + ffi::graphar::property_get_cardinality(&self.0) + } +} + +/// A builder for constructing a [`Property`]. +/// +/// This builder is primarily intended to be used with [`PropertyVec::emplace`] +/// to avoid allocating an intermediate `graphar::Property` on the C++ heap. +/// +/// Defaults: +/// - `is_primary = false` +/// - `is_nullable = true` (matches upstream GraphAr C++ default) +/// - `cardinality = Cardinality::Single` +pub struct PropertyBuilder<S: AsRef<str>> { + name: S, + data_type: DataType, + is_primary: bool, + is_nullable: bool, + cardinality: Cardinality, +} + +impl<S: AsRef<str>> PropertyBuilder<S> { + /// Create a new builder with the given name and data type. + pub fn new(name: S, data_type: DataType) -> Self { + Self { + name, + data_type, + is_primary: false, + is_nullable: true, + cardinality: Cardinality::Single, + } + } + + /// Mark whether this property is a primary key. + pub fn primary_key(mut self, is_primary: bool) -> Self { + self.is_primary = is_primary; + self + } + + /// Mark whether this property is nullable. + pub fn nullable(mut self, is_nullable: bool) -> Self { + self.is_nullable = is_nullable; + self + } + + /// Set the cardinality of this property. + pub fn cardinality(mut self, cardinality: Cardinality) -> Self { + self.cardinality = cardinality; + self + } + + /// Build a [`Property`]. + pub fn build(self) -> Property { + let Self { + name, + data_type, + is_primary, + is_nullable, + cardinality, + } = self; + + let_cxx_string!(name = name.as_ref()); + Property(ffi::graphar::new_property( + &name, + data_type.0, + is_primary, + is_nullable, + cardinality, + )) + } +} + +/// A vector of properties. +/// +/// This is a wrapper around a C++ `std::vector<graphar::Property>`. +pub struct PropertyVec(UniquePtr<CxxVector<ffi::graphar::Property>>); + +impl Default for PropertyVec { + fn default() -> Self { + Self::new() + } +} + +impl Deref for PropertyVec { + type Target = UniquePtr<CxxVector<ffi::graphar::Property>>; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for PropertyVec { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl PropertyVec { + /// Create an empty property vector. + pub fn new() -> Self { + Self(CxxVector::new()) + } + + /// Push a property into the vector. + pub fn push(&mut self, property: Property) { + ffi::graphar::property_vec_push_property(self.0.pin_mut(), property.0); + } + + /// Construct and append a property directly in the underlying C++ vector. + /// + /// Compared to `push(builder.build())`, this avoids allocating an + /// intermediate `graphar::Property` on the C++ heap. + pub fn emplace<S: AsRef<str>>(&mut self, builder: PropertyBuilder<S>) { + let PropertyBuilder { + name, + data_type, + is_primary, + is_nullable, + cardinality, + } = builder; + + let_cxx_string!(name = name.as_ref()); + ffi::graphar::property_vec_emplace_property( + self.0.pin_mut(), + &name, + data_type.0, + is_primary, + is_nullable, + cardinality, + ); + } +} + +/// A group of properties stored in the same file(s). +pub struct PropertyGroup(SharedPtr<ffi::graphar::PropertyGroup>); + +impl PropertyGroup { + /// Create a new property group. + /// + /// The `prefix` is a logical prefix string used by GraphAr (it is not a + /// filesystem path). + pub fn new<S: AsRef<[u8]>>(properties: PropertyVec, file_type: FileType, prefix: S) -> Self { + let_cxx_string!(prefix = prefix); Review Comment: The generic parameter `prefix` with type `S: AsRef<[u8]>` should be converted using `.as_ref()` before being passed to `let_cxx_string!`, consistent with how other generic string-like parameters are handled elsewhere in this file (lines 46, 143, 203). The macro expects a concrete type that implements the required trait, not the generic wrapper. ```suggestion let_cxx_string!(prefix = prefix.as_ref()); ``` ########## rust/src/lib.rs: ########## @@ -15,9 +15,19 @@ // specific language governing permissions and limitations // under the License. -//! Rust bindings for GraphAr data types. +//! Rust bindings for GraphAr. + +#![deny(missing_docs)] + +use cxx::CxxString; mod ffi; +/// GraphAr property. Review Comment: The module documentation comment "GraphAr property." should be more descriptive to match the style of the `types` module documentation on line 28. Consider changing it to "GraphAr property types." or "GraphAr property schema definitions." for consistency and clarity. ```suggestion /// GraphAr property types. ``` -- 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]
