From 58d9439ce491aaf7c688a678b563036ece40c9ea Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Wed, 30 Sep 2026 17:39:18 +0800 Subject: [PATCH] fix(spec): check optional ancestors when binding IS NULL on a nested required field Predicate::bind short-circuited IS NULL to AlwaysFalse (and IS NOT NULL to AlwaysTrue) from the leaf's own `required` flag alone. When an ancestor struct is optional a null parent makes the leaf null, so the shortcut returned wrong rows. Only short-circuit when the field and all ancestors are required. --- crates/iceberg/src/expr/predicate.rs | 108 +++++++++++++++++++++++++- crates/iceberg/src/spec/schema/mod.rs | 77 +++++++++++++++++- 2 files changed, 178 insertions(+), 7 deletions(-) diff --git a/crates/iceberg/src/expr/predicate.rs b/crates/iceberg/src/expr/predicate.rs index f8ea896395..2973a73503 100644 --- a/crates/iceberg/src/expr/predicate.rs +++ b/crates/iceberg/src/expr/predicate.rs @@ -383,16 +383,16 @@ impl Bind for Predicate { }) } Predicate::Unary(expr) => { - let bound_expr = expr.bind(schema, case_sensitive)?; + let bound_expr = expr.bind(schema.clone(), case_sensitive)?; match &bound_expr.op { &PredicateOperator::IsNull => { - if bound_expr.term.field().required { + if schema.is_field_and_ancestors_required(bound_expr.term.field().id) { return Ok(BoundPredicate::AlwaysFalse); } } &PredicateOperator::NotNull => { - if bound_expr.term.field().required { + if schema.is_field_and_ancestors_required(bound_expr.term.field().id) { return Ok(BoundPredicate::AlwaysTrue); } } @@ -821,7 +821,7 @@ mod tests { use crate::expr::Predicate::{AlwaysFalse, AlwaysTrue}; use crate::expr::{Bind, BoundPredicate, Reference}; - use crate::spec::{Datum, NestedField, PrimitiveType, Schema, SchemaRef, Type}; + use crate::spec::{Datum, NestedField, PrimitiveType, Schema, SchemaRef, StructType, Type}; #[test] fn test_logical_or_rewrite_not() { @@ -1060,6 +1060,106 @@ mod tests { test_bound_predicate_serialize_diserialize(bound_expr); } + /// `person: {required|optional} struct`. + fn schema_with_person_struct(person_required: bool) -> SchemaRef { + let inner = Type::Struct(StructType::new(vec![ + NestedField::required(2, "age", Type::Primitive(PrimitiveType::Int)).into(), + NestedField::optional(3, "score", Type::Primitive(PrimitiveType::Int)).into(), + ])); + let person = if person_required { + NestedField::required(1, "person", inner) + } else { + NestedField::optional(1, "person", inner) + }; + Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![person.into()]) + .build() + .unwrap(), + ) + } + + #[test] + fn test_bind_is_null_required_leaf_under_optional_parent_is_kept() { + // `person` is optional, so a null `person` makes `person.age` null even though `age` + // is required. The IS NULL short-circuit must not fire; the predicate is evaluated. + let schema = schema_with_person_struct(false); + let bound = Reference::new("person.age") + .is_null() + .bind(schema, true) + .unwrap(); + assert_eq!(&format!("{bound}"), "person.age IS NULL"); + test_bound_predicate_serialize_diserialize(bound); + } + + #[test] + fn test_bind_is_not_null_required_leaf_under_optional_parent_is_kept() { + let schema = schema_with_person_struct(false); + let bound = Reference::new("person.age") + .is_not_null() + .bind(schema, true) + .unwrap(); + assert_eq!(&format!("{bound}"), "person.age IS NOT NULL"); + test_bound_predicate_serialize_diserialize(bound); + } + + #[test] + fn test_bind_null_predicates_on_leaf_under_required_parent_short_circuit() { + // Whole path required: the leaf can never be null, so the short-circuit still applies. + let schema = schema_with_person_struct(true); + let is_null = Reference::new("person.age") + .is_null() + .bind(schema.clone(), true) + .unwrap(); + assert_eq!(&format!("{is_null}"), "False"); + let is_not_null = Reference::new("person.age") + .is_not_null() + .bind(schema, true) + .unwrap(); + assert_eq!(&format!("{is_not_null}"), "True"); + } + + #[test] + fn test_bind_is_null_required_leaf_under_optional_grandparent_is_kept() { + // `a` (required) > `b` (optional) > `c` (required int). The optional middle struct + // means `a.b.c` can be null, so the walk must reject the short-circuit even though + // both `a` and `c` are required. + let schema = Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required( + 1, + "a", + Type::Struct(StructType::new(vec![ + NestedField::optional( + 2, + "b", + Type::Struct(StructType::new(vec![ + NestedField::required( + 3, + "c", + Type::Primitive(PrimitiveType::Int), + ) + .into(), + ])), + ) + .into(), + ])), + ) + .into(), + ]) + .build() + .unwrap(), + ); + let bound = Reference::new("a.b.c") + .is_null() + .bind(schema, true) + .unwrap(); + assert_eq!(&format!("{bound}"), "a.b.c IS NULL"); + } + #[test] fn test_bind_is_nan() { let schema = table_schema_simple(); diff --git a/crates/iceberg/src/spec/schema/mod.rs b/crates/iceberg/src/spec/schema/mod.rs index c6dcf07079..d433629515 100644 --- a/crates/iceberg/src/spec/schema/mod.rs +++ b/crates/iceberg/src/spec/schema/mod.rs @@ -69,6 +69,7 @@ pub struct Schema { alias_to_id: BiHashMap, id_to_field: HashMap, + id_to_parent: HashMap, name_to_id: HashMap, lowercase_name_to_id: HashMap, @@ -137,10 +138,11 @@ impl SchemaBuilder { let r#struct = StructType::new(self.fields); let id_to_field = index_by_id(&r#struct)?; + let id_to_parent = index_parents(&r#struct)?; Self::validate_identifier_ids( - &r#struct, &id_to_field, + &id_to_parent, self.identifier_field_ids.iter().copied(), )?; @@ -164,6 +166,7 @@ impl SchemaBuilder { identifier_field_ids: self.identifier_field_ids, alias_to_id: self.alias_to_id, id_to_field, + id_to_parent, name_to_id, lowercase_name_to_id, @@ -251,11 +254,10 @@ impl SchemaBuilder { /// - Identifier fields may be nested in structs but cannot be nested within maps or lists. /// - A nested field cannot be used as an identifier field if it is nested in an optional struct, to avoid null values in identifiers. fn validate_identifier_ids( - r#struct: &StructType, id_to_field: &HashMap, + id_to_parent: &HashMap, identifier_field_ids: impl Iterator, ) -> Result<()> { - let id_to_parent = index_parents(r#struct)?; for identifier_field_id in identifier_field_ids { let field = id_to_field.get(&identifier_field_id).ok_or_else(|| { invalid_data!( @@ -397,6 +399,27 @@ impl Schema { self.field_id_to_accessor.get(&field_id).cloned() } + /// Return `true` if `field_id` and every ancestor struct up to the schema root is + /// required. Per the spec a null parent struct implies its leaves are null, so an + /// `IS NULL` / `IS NOT NULL` predicate on a required field can only be short-circuited + /// at bind time when the whole path is required. Returns `false` for an unknown id. + pub(crate) fn is_field_and_ancestors_required(&self, field_id: i32) -> bool { + let Some(field) = self.id_to_field.get(&field_id) else { + return false; + }; + if !field.required { + return false; + } + let mut cur = field_id; + while let Some(parent) = self.id_to_parent.get(&cur) { + match self.id_to_field.get(parent) { + Some(parent_field) if parent_field.required => cur = *parent, + _ => return false, + } + } + true + } + /// Check if this schema is identical to another schema semantically - excluding schema id. pub(crate) fn is_same_schema(&self, other: &SchemaRef) -> bool { self.as_struct().eq(other.as_struct()) @@ -1311,6 +1334,54 @@ table { assert_eq!(0, schema.highest_field_id()); } + #[test] + fn test_is_field_and_ancestors_required() { + // person: optional struct; top: required int; top_opt: optional int + let schema = Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::optional( + 1, + "person", + Struct(StructType::new(vec![ + NestedField::required(2, "age", Primitive(PrimitiveType::Int)).into(), + ])), + ) + .into(), + NestedField::required(3, "top", Primitive(PrimitiveType::Int)).into(), + NestedField::optional(4, "top_opt", Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(); + // Required leaf under an optional parent: a null parent implies a null leaf. + assert!(!schema.is_field_and_ancestors_required(2)); + // The optional parent struct itself. + assert!(!schema.is_field_and_ancestors_required(1)); + // Top-level required / optional. + assert!(schema.is_field_and_ancestors_required(3)); + assert!(!schema.is_field_and_ancestors_required(4)); + // Unknown id. + assert!(!schema.is_field_and_ancestors_required(999)); + + // Whole path required: required struct. + let all_required = Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required( + 1, + "person", + Struct(StructType::new(vec![ + NestedField::required(2, "age", Primitive(PrimitiveType::Int)).into(), + ])), + ) + .into(), + ]) + .build() + .unwrap(); + assert!(all_required.is_field_and_ancestors_required(1)); + assert!(all_required.is_field_and_ancestors_required(2)); + } + #[test] fn test_field_ids_must_be_unique() { let reassigned_schema = Schema::builder()