From 828f62d09c77de3b3bd9ddc80461ea9ced62874e Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Sat, 10 Oct 2026 17:56:25 +0800 Subject: [PATCH] fix: respect ancestor nullability when folding IS NULL/NOT NULL predicates A required field nested in an optional struct, list, or map can be null when its parent is null, so IS NULL and NOT NULL must not be folded for it. Only fold when the field and all of its ancestors are required, matching Java's UnboundPredicate#bindUnaryOperation. --- src/iceberg/expression/predicate.cc | 47 +++++++++++++++++++++++++---- src/iceberg/expression/predicate.h | 2 +- src/iceberg/test/predicate_test.cc | 39 ++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/iceberg/expression/predicate.cc b/src/iceberg/expression/predicate.cc index 9af590f10d..383edbc277 100644 --- a/src/iceberg/expression/predicate.cc +++ b/src/iceberg/expression/predicate.cc @@ -25,11 +25,13 @@ #include "iceberg/expression/expressions.h" #include "iceberg/expression/literal.h" #include "iceberg/result.h" +#include "iceberg/schema.h" #include "iceberg/transform.h" #include "iceberg/type.h" #include "iceberg/util/checked_cast.h" #include "iceberg/util/formatter_internal.h" #include "iceberg/util/macros.h" +#include "iceberg/util/type_util.h" namespace iceberg { @@ -230,7 +232,7 @@ Result> UnboundPredicateImpl::Bind( ICEBERG_ASSIGN_OR_RAISE(auto bound_term, BASE::term()->Bind(schema, case_sensitive)); if (values_.empty()) { - return BindUnaryOperation(std::move(bound_term)); + return BindUnaryOperation(schema, std::move(bound_term)); } if (BASE::op() == Expression::Operation::kIn || @@ -257,30 +259,63 @@ bool StartsWith(const Literal& lhs, const Literal& rhs) { return false; } +/// \brief Returns true if all ancestors of the field with the given ID are required. +/// +/// A required field can still produce null values if it is nested in an optional struct, +/// list, or map. +Result AllAncestorsAreRequired(const Schema& schema, int32_t field_id) { + auto id_to_parent = IndexParents(schema); + auto parent_it = id_to_parent.find(field_id); + while (parent_it != id_to_parent.end()) { + ICEBERG_ASSIGN_OR_RAISE(auto parent, schema.FindFieldById(parent_it->second)); + if (!parent.has_value()) { + return InvalidSchema("Cannot find parent field {} of field {}", parent_it->second, + field_id); + } + if (parent.value().get().optional()) { + return false; + } + parent_it = id_to_parent.find(parent_it->second); + } + return true; +} + } // namespace template Result> UnboundPredicateImpl::BindUnaryOperation( - std::shared_ptr bound_term) const { + const Schema& schema, std::shared_ptr bound_term) const { switch (BASE::op()) { - case Expression::Operation::kIsNull: + case Expression::Operation::kIsNull: { if (!bound_term->MayProduceNull()) { - return Expressions::AlwaysFalse(); + ICEBERG_ASSIGN_OR_RAISE( + bool all_ancestors_required, + AllAncestorsAreRequired(schema, bound_term->reference()->field_id())); + if (all_ancestors_required) { + return Expressions::AlwaysFalse(); + } } if (bound_term->type()->type_id() == TypeId::kUnknown) { return Expressions::AlwaysTrue(); } return BoundUnaryPredicate::Make(Expression::Operation::kIsNull, std::move(bound_term)); - case Expression::Operation::kNotNull: + } + case Expression::Operation::kNotNull: { if (!bound_term->MayProduceNull()) { - return Expressions::AlwaysTrue(); + ICEBERG_ASSIGN_OR_RAISE( + bool all_ancestors_required, + AllAncestorsAreRequired(schema, bound_term->reference()->field_id())); + if (all_ancestors_required) { + return Expressions::AlwaysTrue(); + } } if (bound_term->type()->type_id() == TypeId::kUnknown) { return Expressions::AlwaysFalse(); } return BoundUnaryPredicate::Make(Expression::Operation::kNotNull, std::move(bound_term)); + } case Expression::Operation::kIsNan: case Expression::Operation::kNotNan: if (!IsFloatingType(bound_term->type()->type_id())) { diff --git a/src/iceberg/expression/predicate.h b/src/iceberg/expression/predicate.h index af883fe195..152c6576ed 100644 --- a/src/iceberg/expression/predicate.h +++ b/src/iceberg/expression/predicate.h @@ -150,7 +150,7 @@ class ICEBERG_EXPORT UnboundPredicateImpl : public UnboundPredicate, std::vector values); Result> BindUnaryOperation( - std::shared_ptr bound_term) const; + const Schema& schema, std::shared_ptr bound_term) const; Result> BindLiteralOperation( std::shared_ptr bound_term) const; Result> BindInOperation( diff --git a/src/iceberg/test/predicate_test.cc b/src/iceberg/test/predicate_test.cc index 5d810e6951..13423c6643 100644 --- a/src/iceberg/test/predicate_test.cc +++ b/src/iceberg/test/predicate_test.cc @@ -384,6 +384,45 @@ TEST_F(PredicateTest, UnboundPredicateBindUnaryWithUnknownType) { EXPECT_EQ(bound_not_null->op(), Expression::Operation::kFalse); } +TEST_F(PredicateTest, UnboundPredicateBindUnaryWithNestedStructs) { + // A required field nested in an optional struct can still produce null values + auto nested_schema = std::make_shared( + std::vector{ + SchemaField::MakeOptional(1, "optional_parent", + std::make_shared(std::vector{ + SchemaField::MakeRequired(2, "leaf", int32())})), + SchemaField::MakeRequired(3, "required_parent", + std::make_shared(std::vector{ + SchemaField::MakeRequired(4, "leaf", int32())})), + }, + /*schema_id=*/0); + + auto is_null_optional_parent = Expressions::IsNull("optional_parent.leaf"); + auto bound_is_null_optional = + is_null_optional_parent->Bind(*nested_schema, /*case_sensitive=*/true); + ASSERT_THAT(bound_is_null_optional, IsOk()); + EXPECT_EQ(bound_is_null_optional.value()->op(), Expression::Operation::kIsNull); + + auto not_null_optional_parent = Expressions::NotNull("optional_parent.leaf"); + auto bound_not_null_optional = + not_null_optional_parent->Bind(*nested_schema, /*case_sensitive=*/true); + ASSERT_THAT(bound_not_null_optional, IsOk()); + EXPECT_EQ(bound_not_null_optional.value()->op(), Expression::Operation::kNotNull); + + // A required field with required ancestors can never produce null values + auto is_null_required_parent = Expressions::IsNull("required_parent.leaf"); + auto bound_is_null_required = + is_null_required_parent->Bind(*nested_schema, /*case_sensitive=*/true); + ASSERT_THAT(bound_is_null_required, IsOk()); + EXPECT_EQ(bound_is_null_required.value()->op(), Expression::Operation::kFalse); + + auto not_null_required_parent = Expressions::NotNull("required_parent.leaf"); + auto bound_not_null_required = + not_null_required_parent->Bind(*nested_schema, /*case_sensitive=*/true); + ASSERT_THAT(bound_not_null_required, IsOk()); + EXPECT_EQ(bound_not_null_required.value()->op(), Expression::Operation::kTrue); +} + TEST_F(PredicateTest, UnboundPredicateBindLiteral) { auto equal_pred = Expressions::Equal("age", Literal::Int(25)); auto bound_result = equal_pred->Bind(*schema_, /*case_sensitive=*/true);