From 32b36f82e4377c7fe073d9f9375db674e37fdc9d Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Tue, 29 Sep 2026 05:37:15 +0800 Subject: [PATCH] fix(expression): treat COUNT(*) as referencing no fields in GetReferencedFieldIds ReferenceVisitor::Aggregate unconditionally called aggregate->reference()->field_id(), but BoundAggregate::reference() returns nullptr for aggregates without a term such as COUNT(*), so GetReferencedFieldIds on a bound count(*) dereferenced null and crashed. Skip the insert when there is no reference; the aggregate then correctly contributes no field ids, matching the Java reference. --- src/iceberg/expression/binder.cc | 5 ++++- src/iceberg/test/expression_visitor_test.cc | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/iceberg/expression/binder.cc b/src/iceberg/expression/binder.cc index 9474151d7..9f1b44fbd 100644 --- a/src/iceberg/expression/binder.cc +++ b/src/iceberg/expression/binder.cc @@ -161,7 +161,10 @@ Result ReferenceVisitor::Predicate( Result ReferenceVisitor::Aggregate( const std::shared_ptr& aggregate) { - referenced_field_ids_.insert(aggregate->reference()->field_id()); + // Aggregates without a term (e.g. COUNT(*)) reference no field ids. + if (const auto& reference = aggregate->reference()) { + referenced_field_ids_.insert(reference->field_id()); + } return referenced_field_ids_; } diff --git a/src/iceberg/test/expression_visitor_test.cc b/src/iceberg/test/expression_visitor_test.cc index a3b2c4cad..ca2e039e0 100644 --- a/src/iceberg/test/expression_visitor_test.cc +++ b/src/iceberg/test/expression_visitor_test.cc @@ -805,6 +805,25 @@ TEST_F(ReferenceVisitorTest, Constants) { EXPECT_TRUE(refs_false.empty()); } +TEST_F(ReferenceVisitorTest, CountStar) { + // COUNT(*) has no referenced field; the aggregate's reference() is null, so + // GetReferencedFieldIds must return an empty set instead of dereferencing it. + ICEBERG_UNWRAP_OR_FAIL(auto bound_count_star, Bind(Expressions::CountStar())); + + ICEBERG_UNWRAP_OR_FAIL(auto refs, + ReferenceVisitor::GetReferencedFieldIds(bound_count_star)); + EXPECT_TRUE(refs.empty()); +} + +TEST_F(ReferenceVisitorTest, AggregateWithTerm) { + // An aggregate with a term (e.g. MAX(age)) still references its field, so the + // null guard must not drop the non-null case. age has field id 3. + ICEBERG_UNWRAP_OR_FAIL(auto bound_max, Bind(Expressions::Max("age"))); + + ICEBERG_UNWRAP_OR_FAIL(auto refs, ReferenceVisitor::GetReferencedFieldIds(bound_max)); + EXPECT_THAT(refs, ::testing::UnorderedElementsAre(3)); +} + TEST_F(ReferenceVisitorTest, UnboundPredicate) { auto unbound_pred = Expressions::Equal("name", Literal::String("Alice")); auto result = ReferenceVisitor::GetReferencedFieldIds(unbound_pred);