laskoviymishka commented on code in PR #3190:
URL: https://github.com/apache/iceberg-rust/pull/3190#discussion_r4087479601
##########
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()
+ }
+
+ fn validate(&self) -> Result<()> {
+ for (default_name, default_value) in [
+ ("initial-default", self.initial_default.as_ref()),
+ ("write-default", self.write_default.as_ref()),
+ ] {
+ if let Some(default_value) = default_value {
+ default_value
+ .clone()
+ .try_into_json(&self.field_type)
Review Comment:
`validate()` needs to reject non-null nested-type defaults, mirroring Java's
`castDefault` — right now it only checks the default round-trips through
`try_into_json`, which happily serializes a populated struct/list/map literal.
The spec says nested struct defaults must not contain sub-field values
(null, or a non-null struct with no field values), and Java's `castDefault`
throws on any non-null nested default. So a field built via `AddColumn` with
`initial_default(Literal::Struct(...))` sails through here and gets written
into `schema.json`, and then Java's `SchemaParser.fromJson` throws on it — the
table is unreadable by the reference impl. That's the gap this PR set out to
close.
I'd have `validate()` return `DataInvalid` when `field_type.is_nested()` and
the default is `Some(_)` that isn't an all-`None` struct.
##########
crates/iceberg/src/partitioning.rs:
##########
@@ -139,7 +139,9 @@ pub fn compute_unified_partition_type<'a>(
format!("Missing type for partition field {fid}"),
)
})?;
- Ok(NestedField::optional(fid, name, ty).into())
+ Ok(NestedField::optional(fid, name, ty)
+ .expect("valid nested field")
Review Comment:
This closure returns `Result` and already uses `?` two lines up on
`result_type`, so I'd use `?` here too instead of `.expect()`. It's safe only
because `optional()` sets no default today — but this is a scan-planning path,
and if `validate()` ever grows a check this becomes a live panic with no
build-time signal.
##########
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> {
Review Comment:
None of these six constructors (nor `MapType::optional`/`required`) take a
default, so `validate()` can never fail for them — yet they all now return
`Result`, and that's what drives the 101-file diff and the `.expect("valid
nested field")` sprawl into static `Lazy` schemas, `metadata_columns.rs`, and
even the `rest_catalog_table` example. That's the opposite of the never-panic
instinct.
I'd keep these infallible (`-> Self`) and reserve `Result` for the
`builder()` path + `rebuild` + serde `TryFrom`, where a default can actually be
present — same encapsulation, a fraction of the surface. Unless the uniform
funneling through one validated path was deliberate, in which case it's worth
saying so. wdyt?
##########
crates/iceberg/src/spec/values/literal.rs:
##########
@@ -736,17 +739,19 @@ impl Literal {
let mut id_and_value =
Vec::with_capacity(struct_type.fields().len());
for (value, field) in s.into_iter().zip(struct_type.fields()) {
let json = match value {
- Some(val) => val.try_into_json(&field.field_type)?,
+ Some(val) => val.try_into_json(field.field_type())?,
Review Comment:
The primitive arm just above (line 710), `(_,
PrimitiveLiteral::UInt128(val)) => ...`, matches any `prim_type`, so a
`UInt128` literal validates as a default for `Int`/`String`/`Bool` — unlike the
`Decimal` arm below it, which re-checks the type. Now that `validate()` gates
on `try_into_json`, that wildcard lets a wrong-typed default through. I'd
narrow it to `(PrimitiveType::Uuid, PrimitiveLiteral::UInt128(val))` and let
everything else fall through to the mismatch error.
##########
crates/iceberg/src/spec/schema/id_reassigner.rs:
##########
@@ -41,8 +41,9 @@ impl ReassignFieldIds {
let outer_fields = fields
.into_iter()
.map(|field| {
- try_insert_field(&mut self.old_to_new_id, field.id,
self.next_field_id)?;
- let new_field =
Arc::unwrap_or_clone(field).with_id(self.next_field_id);
+ try_insert_field(&mut self.old_to_new_id, field.id(),
self.next_field_id)?;
+ let field = Arc::unwrap_or_clone(field);
+ let new_field = field.rebuild(self.next_field_id,
field.field_type().clone())?;
Review Comment:
This was an in-place `with_id`; now it deep-clones the whole (possibly
deeply nested) type tree and re-runs `validate()` on every id reassignment,
even though the type isn't changing. I'd add a cheap `pub(crate) fn
with_id(&self, id) -> Self` that swaps only the id and skips validation, and
keep `rebuild()` for the sites that actually change `field_type`. While we're
here, `rebuild()` has no doc comment — worth noting it preserves
name/required/doc/defaults and always re-validates.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -455,39 +455,39 @@ impl Schema {
// `id_to_field` is flattened, so checking each field by its own type
keeps the
// blame on the offending leaf, not its container (mirrors Java's
`lazyIdToField`).
for field in self.id_to_field.values() {
- let min_version = field.field_type.min_format_version();
+ let min_version = field.field_type().min_format_version();
if format_version < min_version {
// Every id in `id_to_field` is also indexed in `id_to_name`;
a miss means
// the schema's indexes are inconsistent (a bug), so surface
it rather than
// guessing an unqualified name.
- let name = self.name_by_field_id(field.id).ok_or_else(|| {
+ let name = self.name_by_field_id(field.id()).ok_or_else(|| {
Error::new(
ErrorKind::Unexpected,
format!(
"Field id {} is missing from the schema's name
index",
- field.id
+ field.id()
),
)
})?;
- problems.push((field.id, format!(
+ problems.push((field.id(), format!(
"Invalid type for {name}: {} is not supported until
{min_version} but format version is {format_version}.",
- field.field_type,
+ field.field_type(),
)));
}
- if let Some(default) = &field.initial_default
+ if let Some(default) = &field.initial_default()
Review Comment:
`initial_default()` already returns `Option<&Literal>`, so the leading `&`
gives `&Option<&Literal>` — leftover from the pre-migration field access. Drop
the `&`.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -583,9 +583,10 @@ impl PartitionSpecBuilder {
),
)
})?;
- let res_type =
partition_field.transform.result_type(&field.field_type)?;
+ let res_type =
partition_field.transform.result_type(field.field_type())?;
let field =
NestedField::optional(partition_field.field_id,
&partition_field.name, res_type)
+ .expect("valid nested field")
Review Comment:
Same as in `partitioning.rs` — the enclosing fn returns `Result` and uses
`?` right above (on `result_type`), so I'd reach for `?` here rather than
`.expect()`.
##########
crates/iceberg/public-api.txt:
##########
@@ -2279,33 +2279,34 @@ pub fn
iceberg::spec::NameMapping::serialize<__S>(&self, __serializer: __S) -> c
impl<'de> serde_core::de::Deserialize<'de> for iceberg::spec::NameMapping
pub fn iceberg::spec::NameMapping::deserialize<__D>(__deserializer: __D) ->
core::result::Result<Self, <__D as serde_core::de::Deserializer>::Error> where
__D: serde_core::de::Deserializer<'de>
pub struct iceberg::spec::NestedField
-pub iceberg::spec::NestedField::doc:
core::option::Option<alloc::string::String>
-pub iceberg::spec::NestedField::field_type:
alloc::boxed::Box<iceberg::spec::Type>
-pub iceberg::spec::NestedField::id: i32
-pub iceberg::spec::NestedField::initial_default:
core::option::Option<iceberg::spec::Literal>
-pub iceberg::spec::NestedField::name: alloc::string::String
-pub iceberg::spec::NestedField::required: bool
-pub iceberg::spec::NestedField::write_default:
core::option::Option<iceberg::spec::Literal>
impl iceberg::spec::NestedField
-pub fn iceberg::spec::NestedField::list_element(id: i32, field_type:
iceberg::spec::Type, required: bool) -> Self
-pub fn iceberg::spec::NestedField::map_key_element(id: i32, field_type:
iceberg::spec::Type) -> Self
-pub fn iceberg::spec::NestedField::map_value_element(id: i32, field_type:
iceberg::spec::Type, required: bool) -> Self
-pub fn iceberg::spec::NestedField::new(id: i32, name: impl
alloc::string::ToString, field_type: iceberg::spec::Type, required: bool) ->
Self
-pub fn iceberg::spec::NestedField::optional(id: i32, name: impl
alloc::string::ToString, field_type: iceberg::spec::Type) -> Self
-pub fn iceberg::spec::NestedField::required(id: i32, name: impl
alloc::string::ToString, field_type: iceberg::spec::Type) -> Self
-pub fn iceberg::spec::NestedField::with_doc(self, doc: impl
alloc::string::ToString) -> Self
-pub fn iceberg::spec::NestedField::with_initial_default(self, value:
iceberg::spec::Literal) -> Self
-pub fn iceberg::spec::NestedField::with_write_default(self, value:
iceberg::spec::Literal) -> Self
+pub fn iceberg::spec::NestedField::doc(&self) -> core::option::Option<&str>
+pub fn iceberg::spec::NestedField::field_type(&self) -> &iceberg::spec::Type
+pub fn iceberg::spec::NestedField::id(&self) -> i32
+pub fn iceberg::spec::NestedField::initial_default(&self) ->
core::option::Option<&iceberg::spec::Literal>
+pub fn iceberg::spec::NestedField::is_required(&self) -> bool
+pub fn iceberg::spec::NestedField::list_element(id: i32, field_type:
iceberg::spec::Type, required: bool) -> iceberg::Result<Self>
+pub fn iceberg::spec::NestedField::map_key_element(id: i32, field_type:
iceberg::spec::Type) -> iceberg::Result<Self>
+pub fn iceberg::spec::NestedField::map_value_element(id: i32, field_type:
iceberg::spec::Type, required: bool) -> iceberg::Result<Self>
+pub fn iceberg::spec::NestedField::name(&self) -> &str
+pub fn iceberg::spec::NestedField::new(id: i32, name: impl
alloc::string::ToString, field_type: iceberg::spec::Type, required: bool) ->
iceberg::Result<Self>
+pub fn iceberg::spec::NestedField::optional(id: i32, name: impl
alloc::string::ToString, field_type: iceberg::spec::Type) ->
iceberg::Result<Self>
+pub fn iceberg::spec::NestedField::required(id: i32, name: impl
alloc::string::ToString, field_type: iceberg::spec::Type) ->
iceberg::Result<Self>
+pub fn iceberg::spec::NestedField::write_default(&self) ->
core::option::Option<&iceberg::spec::Literal>
impl core::clone::Clone for iceberg::spec::NestedField
pub fn iceberg::spec::NestedField::clone(&self) -> iceberg::spec::NestedField
impl core::cmp::Eq for iceberg::spec::NestedField
impl core::cmp::PartialEq for iceberg::spec::NestedField
pub fn iceberg::spec::NestedField::eq(&self, other:
&iceberg::spec::NestedField) -> bool
+impl core::convert::From<iceberg::spec::NestedField> for
iceberg::Result<iceberg::spec::NestedField>
+pub fn iceberg::Result<iceberg::spec::NestedField>::from(field:
iceberg::spec::NestedField) -> Self
impl core::fmt::Debug for iceberg::spec::NestedField
pub fn iceberg::spec::NestedField::fmt(&self, f: &mut
core::fmt::Formatter<'_>) -> core::fmt::Result
impl core::fmt::Display for iceberg::spec::NestedField
pub fn iceberg::spec::NestedField::fmt(&self, f: &mut
core::fmt::Formatter<'_>) -> core::fmt::Result
impl core::marker::StructuralPartialEq for iceberg::spec::NestedField
+impl iceberg::spec::NestedField
+pub fn iceberg::spec::NestedField::builder() -> NestedFieldBuilder<((), (),
(), (), (), (), ())>
Review Comment:
The `typed_builder` type-state tuple leaks into the public-api snapshot, so
any future change to the field count or order will register here as an "API
break" even when no caller-visible contract changed. Not a break now — just
worth a line in the PR description so the `check-public-api` diff isn't misread
later.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1383,6 +1498,91 @@ mod tests {
}
}
+ #[test]
+ fn nested_field_builder_sets_fields() {
+ let field = NestedField::builder()
+ .id(1)
+ .name("count")
+ .required(true)
+ .field_type(Type::Primitive(PrimitiveType::Int))
+ .doc("number of items")
+ .initial_default(Literal::int(1))
+ .write_default(Literal::int(2))
+ .build()
+ .unwrap();
+
+ assert_eq!(field.id(), 1);
+ assert_eq!(field.name(), "count");
+ assert!(field.is_required());
+ assert_eq!(field.field_type(), &Type::Primitive(PrimitiveType::Int));
+ assert_eq!(field.doc(), Some("number of items"));
+ assert_eq!(field.initial_default(), Some(&Literal::int(1)));
+ assert_eq!(field.write_default(), Some(&Literal::int(2)));
+ }
+
+ #[test]
+ fn nested_field_builder_rejects_incompatible_defaults() {
+ let struct_type = Type::Struct(StructType::new(vec![Arc::new(
+ NestedField::required(2, "value",
Type::Primitive(PrimitiveType::String)).unwrap(),
+ )]));
+ let list_type = Type::List(ListType::new(
+ NestedField::list_element(3, Type::Primitive(PrimitiveType::Int),
false)
+ .unwrap()
+ .into(),
+ ));
+ let map_type = Type::Map(
+ MapType::optional(
+ 4,
+ Type::Primitive(PrimitiveType::String),
+ 5,
+ Type::Primitive(PrimitiveType::Int),
+ )
+ .unwrap(),
+ );
+
+ let cases = [
+ (Type::Primitive(PrimitiveType::String), Literal::int(1)),
+ (
+ Type::Primitive(PrimitiveType::Fixed(2)),
+ Literal::binary([1]),
+ ),
+ (
+ struct_type,
+ Literal::Struct(Struct::from_iter([Some(Literal::int(1))])),
+ ),
+ (list_type, Literal::List(vec![Some(Literal::string("one"))])),
+ (
+ map_type,
+ Literal::Map(Map::from([(Literal::int(1),
Some(Literal::int(2)))])),
+ ),
+ ];
+
+ for (field_type, default) in cases {
+ let error = NestedField::builder()
+ .id(1)
+ .name("invalid")
+ .required(false)
+ .field_type(field_type)
+ .initial_default(default)
+ .build()
+ .unwrap_err();
+
+ assert_eq!(error.kind(), ErrorKind::DataInvalid);
+ assert!(error.to_string().contains("field: invalid"));
+ assert!(error.to_string().contains("default: initial-default"));
+ }
+
+ let error = NestedField::builder()
+ .id(1)
+ .name("invalid")
+ .required(false)
+ .field_type(Type::Primitive(PrimitiveType::String))
+ .write_default(Literal::int(1))
Review Comment:
`write_default` rejection is only exercised for this one primitive mismatch,
while `initial_default` gets the full primitive/fixed/struct/list/map sweep —
worth matching the breadth since the PR claims per-type validation. The bigger
gap: there's no end-to-end test driving a bad default through a real call site
(`add_column`, or the Avro/Arrow converter) to confirm the new `Result`
surfaces `DataInvalid` to the caller. Every migrated site is only exercised on
the happy path right now.
--
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]
