dannycjones commented on code in PR #3190:
URL: https://github.com/apache/iceberg-rust/pull/3190#discussion_r4015609536
##########
crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs:
##########
@@ -1845,18 +1845,19 @@ mod tests {
fn build_iceberg_schema_and_field_map() -> Result<(Arc<Schema>,
HashMap<i32, usize>)> {
let iceberg_schema = Schema::builder()
.with_fields([
- Arc::new(NestedField::new(
- 1,
- "col_float",
- Type::Primitive(PrimitiveType::Float),
- false,
- )),
- Arc::new(NestedField::new(
- 2,
- "col_string",
- Type::Primitive(PrimitiveType::String),
- false,
- )),
+ Arc::new(
+ NestedField::new(1, "col_float",
Type::Primitive(PrimitiveType::Float), false)
+ .expect("valid nested field"),
+ ),
+ Arc::new(
+ NestedField::new(
+ 2,
+ "col_string",
+ Type::Primitive(PrimitiveType::String),
+ false,
+ )
+ .expect("valid nested field"),
+ ),
Review Comment:
with these, I am wondering if it would be better to maintain these tests by
constructing a simple vec, and then mapping with `Arc::new` and maybe even
`expect("all fields should be valid")`.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -2250,54 +2283,38 @@ mod tests {
)])),
]));
let iceberg_type = Type::Struct(StructType::new(vec![
- NestedField {
- id: 1,
- doc: None,
- name: "a".to_string(),
- required: true,
- field_type: Box::new(Type::Primitive(PrimitiveType::Long)),
- initial_default: None,
- write_default: None,
- }
- .into(),
- NestedField {
- id: 2,
- doc: None,
- name: "b".to_string(),
- required: false,
- field_type:
Box::new(Type::Primitive(PrimitiveType::String)),
- initial_default: None,
- write_default: None,
- }
- .into(),
+ NestedField::required(1, "a",
Type::Primitive(PrimitiveType::Long))
+ .unwrap()
+ .into(),
+ NestedField::optional(2, "b",
Type::Primitive(PrimitiveType::String))
+ .unwrap()
+ .into(),
]));
assert_eq!(iceberg_type, arrow_type_to_type(&arrow_type).unwrap());
assert_eq!(arrow_type, type_to_arrow_type(&iceberg_type).unwrap());
// initial_default and write_default is ignored
let iceberg_type = Type::Struct(StructType::new(vec![
- NestedField {
- id: 1,
- doc: None,
- name: "a".to_string(),
- required: true,
- field_type: Box::new(Type::Primitive(PrimitiveType::Long)),
- initial_default:
Some(Literal::Primitive(PrimitiveLiteral::Int(114514))),
- write_default: None,
- }
- .into(),
- NestedField {
- id: 2,
- doc: None,
- name: "b".to_string(),
- required: false,
- field_type:
Box::new(Type::Primitive(PrimitiveType::String)),
- initial_default: None,
- write_default:
Some(Literal::Primitive(PrimitiveLiteral::String(
+ NestedField::builder()
+ .id(1)
+ .name("a")
+ .required(true)
+ .field_type(Type::Primitive(PrimitiveType::Long))
+
.initial_default(Literal::Primitive(PrimitiveLiteral::Long(114514)))
Review Comment:
nit: the type here changed - I think it's probably fine actually
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -637,66 +650,113 @@ impl From<NestedField> for SerdeNestedField {
pub type NestedFieldRef = Arc<NestedField>;
impl NestedField {
+ /// Get the id unique in the table schema.
+ pub fn id(&self) -> i32 {
+ self.id
+ }
+
+ /// Get the field name.
+ pub fn name(&self) -> &str {
+ &self.name
+ }
+
+ /// Get whether the field is required.
+ pub fn is_required(&self) -> bool {
+ self.required
+ }
+
+ /// Get the field's data type.
+ pub fn field_type(&self) -> &Type {
+ &self.field_type
+ }
+
+ /// Get the field's documentation string.
+ pub fn doc(&self) -> Option<&str> {
+ self.doc.as_deref()
+ }
+
+ /// Get the field's initial default value.
+ pub fn initial_default(&self) -> Option<&Literal> {
+ self.initial_default.as_ref()
+ }
+
+ /// Get the field's write default value.
+ pub fn write_default(&self) -> Option<&Literal> {
+ self.write_default.as_ref()
+ }
+
/// Construct a new field.
- pub fn new(id: i32, name: impl ToString, field_type: Type, required: bool)
-> Self {
- Self {
- id,
- name: name.to_string(),
- required,
- field_type: Box::new(field_type),
- doc: None,
- initial_default: None,
- write_default: None,
- }
+ pub fn new(id: i32, name: impl ToString, field_type: Type, required: bool)
-> Result<Self> {
+ Self::builder()
+ .id(id)
+ .name(name.to_string())
+ .required(required)
+ .field_type(field_type)
+ .build()
}
/// Construct a required field.
- pub fn required(id: i32, name: impl ToString, field_type: Type) -> Self {
+ pub fn required(id: i32, name: impl ToString, field_type: Type) ->
Result<Self> {
Self::new(id, name, field_type, true)
}
/// Construct an optional field.
- pub fn optional(id: i32, name: impl ToString, field_type: Type) -> Self {
+ pub fn optional(id: i32, name: impl ToString, field_type: Type) ->
Result<Self> {
Self::new(id, name, field_type, false)
}
/// Construct list type's element field.
- pub fn list_element(id: i32, field_type: Type, required: bool) -> Self {
+ pub fn list_element(id: i32, field_type: Type, required: bool) ->
Result<Self> {
Self::new(id, LIST_FIELD_NAME, field_type, required)
}
/// Construct map type's key field.
- pub fn map_key_element(id: i32, field_type: Type) -> Self {
+ pub fn map_key_element(id: i32, field_type: Type) -> Result<Self> {
Self::required(id, MAP_KEY_FIELD_NAME, field_type)
}
/// Construct map type's value field.
- pub fn map_value_element(id: i32, field_type: Type, required: bool) ->
Self {
+ pub fn map_value_element(id: i32, field_type: Type, required: bool) ->
Result<Self> {
Self::new(id, MAP_VALUE_FIELD_NAME, field_type, required)
}
- /// Set the field's doc.
- pub fn with_doc(mut self, doc: impl ToString) -> Self {
- self.doc = Some(doc.to_string());
- self
- }
-
- /// Set the field's initial default value.
- pub fn with_initial_default(mut self, value: Literal) -> Self {
- self.initial_default = Some(value);
- self
- }
+ pub(crate) fn rebuild(&self, id: i32, field_type: Type) -> Result<Self> {
+ Self::builder()
+ .id(id)
+ .name(self.name.clone())
+ .required(self.required)
+ .field_type(field_type)
+ .doc_opt(self.doc.clone())
+ .initial_default_opt(self.initial_default.clone())
+ .write_default_opt(self.write_default.clone())
+ .build()
+ }
Review Comment:
Maybe some Rustdoc to explain in what scenario we may want to rebuild with a
new ID or field type.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1142,30 +1230,43 @@ mod tests {
"#;
let struct_type = Type::Struct(StructType::new(vec![
- NestedField::required(1, "id",
Type::Primitive(PrimitiveType::Uuid))
-
.with_initial_default(Literal::Primitive(PrimitiveLiteral::UInt128(
+ NestedField::builder()
+ .id(1)
+ .name("id")
+ .required(true)
+ .field_type(Type::Primitive(PrimitiveType::Uuid))
+ .initial_default(Literal::Primitive(PrimitiveLiteral::UInt128(
Uuid::parse_str("0db3e2a8-9d1d-42b9-aa7b-74ebe558dceb")
.unwrap()
.as_u128(),
)))
-
.with_write_default(Literal::Primitive(PrimitiveLiteral::UInt128(
+ .write_default(Literal::Primitive(PrimitiveLiteral::UInt128(
Uuid::parse_str("ec5911be-b0a7-458c-8438-c9a3e53cffae")
.unwrap()
.as_u128(),
)))
Review Comment:
I'm wondering if we can make some ergonomic improvements to some other APIs,
by adopting `Into<Literal>` in `write_default` setter.
```suggestion
.write_default(PrimitiveLiteral::UInt128(
Uuid::parse_str("ec5911be-b0a7-458c-8438-c9a3e53cffae")
.unwrap()
.as_u128(),
))
```
This can be a follow-up, and it would be opt-in so new tests and changes
could pick it up.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -637,66 +650,113 @@ impl From<NestedField> for SerdeNestedField {
pub type NestedFieldRef = Arc<NestedField>;
impl NestedField {
+ /// Get the id unique in the table schema.
+ pub fn id(&self) -> i32 {
+ self.id
+ }
+
+ /// Get the field name.
+ pub fn name(&self) -> &str {
+ &self.name
+ }
+
+ /// Get whether the field is required.
+ pub fn is_required(&self) -> bool {
+ self.required
+ }
+
+ /// Get the field's data type.
+ pub fn field_type(&self) -> &Type {
+ &self.field_type
+ }
+
+ /// Get the field's documentation string.
+ pub fn doc(&self) -> Option<&str> {
+ self.doc.as_deref()
+ }
+
+ /// Get the field's initial default value.
+ pub fn initial_default(&self) -> Option<&Literal> {
+ self.initial_default.as_ref()
+ }
+
+ /// Get the field's write default value.
+ pub fn write_default(&self) -> Option<&Literal> {
+ self.write_default.as_ref()
+ }
+
/// Construct a new field.
- pub fn new(id: i32, name: impl ToString, field_type: Type, required: bool)
-> Self {
- Self {
- id,
- name: name.to_string(),
- required,
- field_type: Box::new(field_type),
- doc: None,
- initial_default: None,
- write_default: None,
- }
+ pub fn new(id: i32, name: impl ToString, field_type: Type, required: bool)
-> Result<Self> {
+ Self::builder()
+ .id(id)
+ .name(name.to_string())
+ .required(required)
+ .field_type(field_type)
+ .build()
}
/// Construct a required field.
- pub fn required(id: i32, name: impl ToString, field_type: Type) -> Self {
+ pub fn required(id: i32, name: impl ToString, field_type: Type) ->
Result<Self> {
Self::new(id, name, field_type, true)
}
/// Construct an optional field.
- pub fn optional(id: i32, name: impl ToString, field_type: Type) -> Self {
+ pub fn optional(id: i32, name: impl ToString, field_type: Type) ->
Result<Self> {
Self::new(id, name, field_type, false)
}
Review Comment:
Would it be ergonomic here to return a builder, preconfigured as optional or
required?
Then call sites are one of these:
```rust
let f0 = NestedField::required(0, "f0", PrimitiveType::Int).build().unwrap()
let f1 = NestedField::required(1, "f1", PrimitiveType::Int)
.write_default(Literal::Primitive(PrimitiveLiteral::Int(5)))
.build()
.unwrap()
```
--
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]
