Skip to content

fix(spec): check optional ancestors when binding IS NULL on a nested required field #3306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LuciferYang wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(spec): check optional ancestors when binding IS NULL on a nested required field #3306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 104 additions & 4 deletions crates/iceberg/src/expr/predicate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -1060,6 +1060,106 @@ mod tests {
test_bound_predicate_serialize_diserialize(bound_expr);
}

/// `person: {required|optional} struct<age: required int, score: optional int>`.
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();
Expand Down
77 changes: 74 additions & 3 deletions crates/iceberg/src/spec/schema/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ pub struct Schema {

alias_to_id: BiHashMap<String, i32>,
id_to_field: HashMap<i32, NestedFieldRef>,
id_to_parent: HashMap<i32, i32>,

name_to_id: HashMap<String, i32>,
lowercase_name_to_id: HashMap<String, i32>,
Expand Down Expand Up @@ -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(),
)?;

Expand All @@ -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,
Expand Down Expand Up @@ -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<i32, NestedFieldRef>,
id_to_parent: &HashMap<i32, i32>,
identifier_field_ids: impl Iterator<Item = i32>,
) -> 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!(
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -1311,6 +1334,54 @@ table {
assert_eq!(0, schema.highest_field_id());
}

#[test]
fn test_is_field_and_ancestors_required() {
// person: optional struct<age: required int>; 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<age: required int>.
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()
Expand Down
Loading