Skip to content

Commit a2e276c

Browse files
author
yinzhengsun
committed
fix comments
1 parent 767f701 commit a2e276c

5 files changed

Lines changed: 131 additions & 68 deletions

File tree

‎src/iceberg/expression/predicate.cc‎

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@
2525
#include "iceberg/expression/expressions.h"
2626
#include "iceberg/expression/literal.h"
2727
#include "iceberg/result.h"
28-
#include "iceberg/row/struct_like.h"
2928
#include "iceberg/schema.h"
3029
#include "iceberg/transform.h"
3130
#include "iceberg/type.h"
3231
#include "iceberg/util/checked_cast.h"
3332
#include "iceberg/util/formatter_internal.h"
3433
#include "iceberg/util/macros.h"
34+
#include "iceberg/util/type_util.h"
3535

3636
namespace iceberg {
3737

@@ -249,19 +249,6 @@ bool IsFloatingType(TypeId type) {
249249
return type == TypeId::kFloat || type == TypeId::kDouble;
250250
}
251251

252-
bool AllAncestorsRequired(const StructType& struct_type, const BoundReference& ref) {
253-
const auto& path = ref.accessor().position_path();
254-
const StructType* parent = &struct_type;
255-
for (size_t i = 0; i + 1 < path.size(); ++i) {
256-
const auto& field = parent->fields()[path[i]];
257-
if (field.optional()) {
258-
return false;
259-
}
260-
parent = &internal::checked_cast<const StructType&>(*field.type());
261-
}
262-
return true;
263-
}
264-
265252
bool StartsWith(const Literal& lhs, const Literal& rhs) {
266253
const auto& lhs_value = lhs.value();
267254
const auto& rhs_value = rhs.value();
@@ -272,32 +259,63 @@ bool StartsWith(const Literal& lhs, const Literal& rhs) {
272259
return false;
273260
}
274261

262+
/// \brief Returns true if all ancestors of the field with the given ID are required.
263+
///
264+
/// A required field can still produce null values if it is nested in an optional struct,
265+
/// list, or map.
266+
Result<bool> AllAncestorsAreRequired(const Schema& schema, int32_t field_id) {
267+
auto id_to_parent = IndexParents(schema);
268+
auto parent_it = id_to_parent.find(field_id);
269+
while (parent_it != id_to_parent.end()) {
270+
ICEBERG_ASSIGN_OR_RAISE(auto parent, schema.FindFieldById(parent_it->second));
271+
if (!parent.has_value()) {
272+
return InvalidSchema("Cannot find parent field {} of field {}", parent_it->second,
273+
field_id);
274+
}
275+
if (parent.value().get().optional()) {
276+
return false;
277+
}
278+
parent_it = id_to_parent.find(parent_it->second);
279+
}
280+
return true;
281+
}
282+
275283
} // namespace
276284

277285
template <typename B>
278286
Result<std::shared_ptr<Expression>> UnboundPredicateImpl<B>::BindUnaryOperation(
279-
const StructType& struct_type, std::shared_ptr<B> bound_term) const {
287+
const Schema& schema, std::shared_ptr<B> bound_term) const {
280288
switch (BASE::op()) {
281-
case Expression::Operation::kIsNull:
282-
if (!bound_term->MayProduceNull() &&
283-
AllAncestorsRequired(struct_type, *bound_term->reference())) {
284-
return Expressions::AlwaysFalse();
289+
case Expression::Operation::kIsNull: {
290+
if (!bound_term->MayProduceNull()) {
291+
ICEBERG_ASSIGN_OR_RAISE(
292+
bool all_ancestors_required,
293+
AllAncestorsAreRequired(schema, bound_term->reference()->field_id()));
294+
if (all_ancestors_required) {
295+
return Expressions::AlwaysFalse();
296+
}
285297
}
286298
if (bound_term->type()->type_id() == TypeId::kUnknown) {
287299
return Expressions::AlwaysTrue();
288300
}
289301
return BoundUnaryPredicate::Make(Expression::Operation::kIsNull,
290302
std::move(bound_term));
291-
case Expression::Operation::kNotNull:
292-
if (!bound_term->MayProduceNull() &&
293-
AllAncestorsRequired(struct_type, *bound_term->reference())) {
294-
return Expressions::AlwaysTrue();
303+
}
304+
case Expression::Operation::kNotNull: {
305+
if (!bound_term->MayProduceNull()) {
306+
ICEBERG_ASSIGN_OR_RAISE(
307+
bool all_ancestors_required,
308+
AllAncestorsAreRequired(schema, bound_term->reference()->field_id()));
309+
if (all_ancestors_required) {
310+
return Expressions::AlwaysTrue();
311+
}
295312
}
296313
if (bound_term->type()->type_id() == TypeId::kUnknown) {
297314
return Expressions::AlwaysFalse();
298315
}
299316
return BoundUnaryPredicate::Make(Expression::Operation::kNotNull,
300317
std::move(bound_term));
318+
}
301319
case Expression::Operation::kIsNan:
302320
case Expression::Operation::kNotNan:
303321
if (!IsFloatingType(bound_term->type()->type_id())) {

‎src/iceberg/expression/predicate.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ class ICEBERG_EXPORT UnboundPredicateImpl : public UnboundPredicate,
150150
std::vector<Literal> values);
151151

152152
Result<std::shared_ptr<Expression>> BindUnaryOperation(
153-
const StructType& struct_type, std::shared_ptr<B> bound_term) const;
153+
const Schema& schema, std::shared_ptr<B> bound_term) const;
154154
Result<std::shared_ptr<Expression>> BindLiteralOperation(
155155
std::shared_ptr<B> bound_term) const;
156156
Result<std::shared_ptr<Expression>> BindInOperation(

‎src/iceberg/expression/residual_evaluator.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "iceberg/expression/expression.h"
2323
#include "iceberg/expression/expression_visitor.h"
2424
#include "iceberg/expression/predicate.h"
25+
#include "iceberg/expression/rewrite_not.h"
2526
#include "iceberg/partition_spec.h"
2627
#include "iceberg/row/struct_like.h"
2728
#include "iceberg/schema.h"
@@ -328,6 +329,7 @@ Result<std::unique_ptr<ResidualEvaluator>> ResidualEvaluator::Unpartitioned(
328329
Result<std::unique_ptr<ResidualEvaluator>> ResidualEvaluator::Make(
329330
std::shared_ptr<Expression> expr, const PartitionSpec& spec, const Schema& schema,
330331
bool case_sensitive) {
332+
ICEBERG_ASSIGN_OR_RAISE(expr, RewriteNot::Visit(std::move(expr)));
331333
if (spec.fields().empty()) {
332334
return Unpartitioned(std::move(expr));
333335
}

‎src/iceberg/test/predicate_test.cc‎

Lines changed: 39 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
#include <limits>
2323
#include <memory>
24-
#include <utility>
2524

2625
#include "iceberg/expression/expressions.h"
2726
#include "iceberg/schema.h"
@@ -367,49 +366,6 @@ TEST_F(PredicateTest, UnboundPredicateBindUnary) {
367366
EXPECT_EQ(bound_is_null->op(), Expression::Operation::kFalse);
368367
}
369368

370-
TEST_F(PredicateTest, BindNestedNullPredicates) {
371-
for (auto [outer_optional, inner_optional] :
372-
{std::pair{false, false}, std::pair{true, false}, std::pair{false, true}}) {
373-
auto inner = std::make_shared<StructType>(
374-
std::vector<SchemaField>{SchemaField::MakeRequired(4, "leaf", int32())});
375-
auto outer = std::make_shared<StructType>(
376-
std::vector<SchemaField>{SchemaField(3, "middle", inner, inner_optional)});
377-
Schema schema(
378-
std::vector<SchemaField>{SchemaField::MakeOptional(1, "unrelated", int32()),
379-
SchemaField(2, "key", outer, outer_optional)});
380-
SCOPED_TRACE(schema.ToString());
381-
bool nullable = outer_optional || inner_optional;
382-
ICEBERG_UNWRAP_OR_FAIL(auto is_null,
383-
Expressions::IsNull("key.middle.leaf")->Bind(schema, true));
384-
ICEBERG_UNWRAP_OR_FAIL(auto not_null,
385-
Expressions::NotNull("key.middle.leaf")->Bind(schema, true));
386-
EXPECT_EQ(is_null->op(),
387-
nullable ? Expression::Operation::kIsNull : Expression::Operation::kFalse);
388-
EXPECT_EQ(not_null->op(),
389-
nullable ? Expression::Operation::kNotNull : Expression::Operation::kTrue);
390-
}
391-
}
392-
393-
TEST_F(PredicateTest, BindTransformedNestedNullPredicates) {
394-
for (bool parent_optional : {false, true}) {
395-
Schema schema(std::vector<SchemaField>{SchemaField(
396-
1, "key",
397-
std::make_shared<StructType>(
398-
std::vector<SchemaField>{SchemaField::MakeRequired(2, "nested", int32())}),
399-
parent_optional)});
400-
auto truncate = Expressions::Truncate("key.nested", 10);
401-
ICEBERG_UNWRAP_OR_FAIL(
402-
auto is_null, Expressions::IsNull<BoundTransform>(truncate)->Bind(schema, true));
403-
ICEBERG_UNWRAP_OR_FAIL(
404-
auto not_null,
405-
Expressions::NotNull<BoundTransform>(truncate)->Bind(schema, true));
406-
EXPECT_EQ(is_null->op(), parent_optional ? Expression::Operation::kIsNull
407-
: Expression::Operation::kFalse);
408-
EXPECT_EQ(not_null->op(), parent_optional ? Expression::Operation::kNotNull
409-
: Expression::Operation::kTrue);
410-
}
411-
}
412-
413369
TEST_F(PredicateTest, UnboundPredicateBindUnaryWithUnknownType) {
414370
// Test IS NULL on unknown type field - should return AlwaysTrue
415371
auto is_null_unknown = Expressions::IsNull("always_null"); // always_null is unknown
@@ -428,6 +384,45 @@ TEST_F(PredicateTest, UnboundPredicateBindUnaryWithUnknownType) {
428384
EXPECT_EQ(bound_not_null->op(), Expression::Operation::kFalse);
429385
}
430386

387+
TEST_F(PredicateTest, UnboundPredicateBindUnaryWithNestedStructs) {
388+
// A required field nested in an optional struct can still produce null values
389+
auto nested_schema = std::make_shared<Schema>(
390+
std::vector<SchemaField>{
391+
SchemaField::MakeOptional(1, "optional_parent",
392+
std::make_shared<StructType>(std::vector<SchemaField>{
393+
SchemaField::MakeRequired(2, "leaf", int32())})),
394+
SchemaField::MakeRequired(3, "required_parent",
395+
std::make_shared<StructType>(std::vector<SchemaField>{
396+
SchemaField::MakeRequired(4, "leaf", int32())})),
397+
},
398+
/*schema_id=*/0);
399+
400+
auto is_null_optional_parent = Expressions::IsNull("optional_parent.leaf");
401+
auto bound_is_null_optional =
402+
is_null_optional_parent->Bind(*nested_schema, /*case_sensitive=*/true);
403+
ASSERT_THAT(bound_is_null_optional, IsOk());
404+
EXPECT_EQ(bound_is_null_optional.value()->op(), Expression::Operation::kIsNull);
405+
406+
auto not_null_optional_parent = Expressions::NotNull("optional_parent.leaf");
407+
auto bound_not_null_optional =
408+
not_null_optional_parent->Bind(*nested_schema, /*case_sensitive=*/true);
409+
ASSERT_THAT(bound_not_null_optional, IsOk());
410+
EXPECT_EQ(bound_not_null_optional.value()->op(), Expression::Operation::kNotNull);
411+
412+
// A required field with required ancestors can never produce null values
413+
auto is_null_required_parent = Expressions::IsNull("required_parent.leaf");
414+
auto bound_is_null_required =
415+
is_null_required_parent->Bind(*nested_schema, /*case_sensitive=*/true);
416+
ASSERT_THAT(bound_is_null_required, IsOk());
417+
EXPECT_EQ(bound_is_null_required.value()->op(), Expression::Operation::kFalse);
418+
419+
auto not_null_required_parent = Expressions::NotNull("required_parent.leaf");
420+
auto bound_not_null_required =
421+
not_null_required_parent->Bind(*nested_schema, /*case_sensitive=*/true);
422+
ASSERT_THAT(bound_not_null_required, IsOk());
423+
EXPECT_EQ(bound_not_null_required.value()->op(), Expression::Operation::kTrue);
424+
}
425+
431426
TEST_F(PredicateTest, UnboundPredicateBindLiteral) {
432427
auto equal_pred = Expressions::Equal("age", Literal::Int(25));
433428
auto bound_result = equal_pred->Bind(*schema_, /*case_sensitive=*/true);

‎src/iceberg/test/residual_evaluator_test.cc‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,54 @@ TEST_F(ResidualEvaluatorTest, CaseInsensitiveIdentityTransformResiduals) {
219219
EXPECT_EQ(residual4->op(), Expression::Operation::kFalse);
220220
}
221221

222+
TEST_F(ResidualEvaluatorTest, ComparisonWithNullValue) {
223+
auto schema = std::make_shared<Schema>(
224+
std::vector<SchemaField>{SchemaField::MakeOptional(50, "key", int32()),
225+
SchemaField::MakeOptional(51, "value", int32())});
226+
227+
PartitionField pt_field(50, 1000, "key", Transform::Identity());
228+
ICEBERG_UNWRAP_OR_FAIL(auto spec_unique,
229+
PartitionSpec::Make(*schema, 0, {pt_field}, false));
230+
auto spec = std::shared_ptr<PartitionSpec>(spec_unique.release());
231+
232+
// NOT(key < 100) OR value = 5
233+
auto expr =
234+
Expressions::Or(Expressions::Not(Expressions::LessThan("key", Literal::Int(100))),
235+
Expressions::Equal("value", Literal::Int(5)));
236+
ICEBERG_UNWRAP_OR_FAIL(auto evaluator,
237+
ResidualEvaluator::Make(expr, *spec, *schema, true));
238+
239+
// A null partition value must not satisfy NOT(key < 100); otherwise the residual
240+
// folds to true and drops value = 5, disagreeing with Evaluator.
241+
PartitionValues null_partition(Literal::Null(int32()));
242+
ICEBERG_UNWRAP_OR_FAIL(auto null_residual, evaluator->ResidualFor(null_partition));
243+
ASSERT_TRUE(null_residual->is_unbound_predicate());
244+
auto unbound = std::dynamic_pointer_cast<UnboundPredicate>(null_residual);
245+
ASSERT_NE(unbound, nullptr);
246+
EXPECT_EQ(unbound->op(), Expression::Operation::kEq);
247+
EXPECT_EQ(unbound->reference()->name(), "value");
248+
249+
PartitionValues below(Literal::Int(50));
250+
ICEBERG_UNWRAP_OR_FAIL(auto below_residual, evaluator->ResidualFor(below));
251+
ASSERT_TRUE(below_residual->is_unbound_predicate());
252+
EXPECT_EQ(below_residual->op(), Expression::Operation::kEq);
253+
254+
PartitionValues above(Literal::Int(150));
255+
ICEBERG_UNWRAP_OR_FAIL(auto above_residual, evaluator->ResidualFor(above));
256+
EXPECT_EQ(above_residual->op(), Expression::Operation::kTrue);
257+
258+
ICEBERG_UNWRAP_OR_FAIL(
259+
auto unpartitioned,
260+
ResidualEvaluator::Make(expr, *PartitionSpec::Unpartitioned(), *schema, true));
261+
PartitionValues empty_partition;
262+
ICEBERG_UNWRAP_OR_FAIL(auto unpartitioned_residual,
263+
unpartitioned->ResidualFor(empty_partition));
264+
ASSERT_EQ(unpartitioned_residual->op(), Expression::Operation::kOr);
265+
auto unpartitioned_or = std::dynamic_pointer_cast<Or>(unpartitioned_residual);
266+
ASSERT_NE(unpartitioned_or, nullptr);
267+
EXPECT_EQ(unpartitioned_or->left()->op(), Expression::Operation::kGtEq);
268+
}
269+
222270
TEST_F(ResidualEvaluatorTest, UnpartitionedResiduals) {
223271
std::vector<std::shared_ptr<Expression>> expressions = {
224272
Expressions::AlwaysTrue(),

0 commit comments

Comments
 (0)