Skip to content

fix: respect ancestor nullability when folding IS NULL/NOT NULL predicates #995

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

Merged
wgtmac merged 1 commit into from
Oct 13, 2026
Merged

fix: respect ancestor nullability when folding IS NULL/NOT NULL predicates #995

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
47 changes: 41 additions & 6 deletions src/iceberg/expression/predicate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -230,7 +232,7 @@ Result<std::shared_ptr<Expression>> UnboundPredicateImpl<B>::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 ||
Expand All @@ -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<bool> 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,

@advancedxy advancedxy Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. It's not an invalid schema error? How about invalid argument error?

@wgtmac wgtmac Oct 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, invalid argument is used for a wrong schema or field_id input, but invalid schema here is more precise because the schema is unexpected to miss field_id.

field_id);
}
if (parent.value().get().optional()) {
return false;
}
parent_it = id_to_parent.find(parent_it->second);
}
return true;
}

} // namespace

template <typename B>
Result<std::shared_ptr<Expression>> UnboundPredicateImpl<B>::BindUnaryOperation(
std::shared_ptr<B> bound_term) const {
const Schema& schema, std::shared_ptr<B> 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())) {
Expand Down
2 changes: 1 addition & 1 deletion src/iceberg/expression/predicate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ class ICEBERG_EXPORT UnboundPredicateImpl : public UnboundPredicate,
std::vector<Literal> values);

Result<std::shared_ptr<Expression>> BindUnaryOperation(
std::shared_ptr<B> bound_term) const;
const Schema& schema, std::shared_ptr<B> bound_term) const;
Result<std::shared_ptr<Expression>> BindLiteralOperation(
std::shared_ptr<B> bound_term) const;
Result<std::shared_ptr<Expression>> BindInOperation(
Expand Down
39 changes: 39 additions & 0 deletions src/iceberg/test/predicate_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Schema>(
std::vector<SchemaField>{
SchemaField::MakeOptional(1, "optional_parent",
std::make_shared<StructType>(std::vector<SchemaField>{
SchemaField::MakeRequired(2, "leaf", int32())})),
SchemaField::MakeRequired(3, "required_parent",
std::make_shared<StructType>(std::vector<SchemaField>{
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);
Expand Down
Loading