minor polish

Author: wgtmac

src/iceberg/expression/aggregate.cc

@@ -42,6 +42,19 @@ std::shared_ptr<PrimitiveType> GetPrimitiveType(const BoundTerm& term) {
   return internal::checked_pointer_cast<PrimitiveType>(term.type());
 }
 
+/// \brief A single-field StructLike that wraps a Literal
+class SingleValueStructLike : public StructLike {
+ public:
+  explicit SingleValueStructLike(Literal literal) : literal_(std::move(literal)) {}
+
+  Result<Scalar> GetField(size_t) const override { return LiteralToScalar(literal_); }
+
+  size_t num_fields() const override { return 1; }
+
+ private:
+  Literal literal_;
+};
+
 Result<Literal> EvaluateBoundTerm(const BoundTerm& term,
                                   const std::optional<std::vector<uint8_t>>& bound) {
   auto ptype = GetPrimitiveType(term);
@@ -111,6 +124,7 @@ class MaxAggregator : public BoundAggregate::Aggregator {
 
     if (auto ordering = value <=> current_;
         ordering == std::partial_ordering::unordered) {
+      valid_ = false;
       return InvalidArgument("Cannot compare literal {} with current value {}",
                              value.ToString(), current_.ToString());
     } else if (ordering == std::partial_ordering::greater) {
@@ -140,6 +154,7 @@ class MaxAggregator : public BoundAggregate::Aggregator {
 
     if (auto ordering = value <=> current_;
         ordering == std::partial_ordering::unordered) {
+      valid_ = false;
       return InvalidArgument("Cannot compare literal {} with current value {}",
                              value.ToString(), current_.ToString());
     } else if (ordering == std::partial_ordering::greater) {
@@ -181,6 +196,7 @@ class MinAggregator : public BoundAggregate::Aggregator {
 
     if (auto ordering = value <=> current_;
         ordering == std::partial_ordering::unordered) {
+      valid_ = false;
       return InvalidArgument("Cannot compare literal {} with current value {}",
                              value.ToString(), current_.ToString());
     } else if (ordering == std::partial_ordering::less) {
@@ -209,6 +225,7 @@ class MinAggregator : public BoundAggregate::Aggregator {
 
     if (auto ordering = value <=> current_;
         ordering == std::partial_ordering::unordered) {
+      valid_ = false;
       return InvalidArgument("Cannot compare literal {} with current value {}",
                              value.ToString(), current_.ToString());
     } else if (ordering == std::partial_ordering::less) {

src/iceberg/expression/aggregate.h

@@ -111,12 +111,13 @@ class ICEBERG_EXPORT BoundAggregate : public Aggregate<BoundTerm>, public Bound
 
     virtual Status Update(const DataFile& file) = 0;
 
-    virtual bool IsValid() const { return true; }
+    /// \brief Whether the aggregator is still valid.
+    virtual bool IsValid() const = 0;
 
     /// \brief Get the result of the aggregation.
     /// \return The result of the aggregation.
     /// \note It is an undefined behavior to call this method if any previous Update call
-    /// has returned an error.
+    /// has returned an error or if IsValid() returns false.
     virtual Literal GetResult() const = 0;
   };
 
@@ -127,6 +128,7 @@ class ICEBERG_EXPORT BoundAggregate : public Aggregate<BoundTerm>, public Bound
   }
 
   Result<Literal> Evaluate(const StructLike& data) const override = 0;
+
   virtual Result<Literal> Evaluate(const DataFile& file) const = 0;
 
   /// \brief Whether metrics in the data file are sufficient to evaluate.
@@ -146,7 +148,7 @@ class ICEBERG_EXPORT BoundAggregate : public Aggregate<BoundTerm>, public Bound
 /// \brief Base class for COUNT aggregates.
 class ICEBERG_EXPORT CountAggregate : public BoundAggregate {
  public:
-  Result<Literal> Evaluate(const StructLike& data) const final;
+  Result<Literal> Evaluate(const StructLike& data) const override;
   Result<Literal> Evaluate(const DataFile& file) const override;
 
   std::unique_ptr<Aggregator> NewAggregator() const override;
@@ -156,8 +158,6 @@ class ICEBERG_EXPORT CountAggregate : public BoundAggregate {
   /// \brief Count using metrics from a data file.
   virtual Result<int64_t> CountFor(const DataFile& file) const = 0;
 
-  bool HasValue(const DataFile& file) const override = 0;
-
  protected:
   CountAggregate(Expression::Operation op, std::shared_ptr<BoundTerm> term)
       : BoundAggregate(op, std::move(term)) {}
@@ -171,7 +171,6 @@ class ICEBERG_EXPORT CountNonNullAggregate : public CountAggregate {
 
   Result<int64_t> CountFor(const StructLike& data) const override;
   Result<int64_t> CountFor(const DataFile& file) const override;
-
   bool HasValue(const DataFile& file) const override;
 
  private:
@@ -186,7 +185,6 @@ class ICEBERG_EXPORT CountNullAggregate : public CountAggregate {
 
   Result<int64_t> CountFor(const StructLike& data) const override;
   Result<int64_t> CountFor(const DataFile& file) const override;
-
   bool HasValue(const DataFile& file) const override;
 
  private:
@@ -200,7 +198,6 @@ class ICEBERG_EXPORT CountStarAggregate : public CountAggregate {
 
   Result<int64_t> CountFor(const StructLike& data) const override;
   Result<int64_t> CountFor(const DataFile& file) const override;
-
   bool HasValue(const DataFile& file) const override;
 
  private:
@@ -213,12 +210,11 @@ class ICEBERG_EXPORT MaxAggregate : public BoundAggregate {
   static std::shared_ptr<MaxAggregate> Make(std::shared_ptr<BoundTerm> term);
 
   Result<Literal> Evaluate(const StructLike& data) const override;
-  Result<Literal> Evaluate(const DataFile& file) const final;
+  Result<Literal> Evaluate(const DataFile& file) const override;
+  bool HasValue(const DataFile& file) const override;
 
   std::unique_ptr<Aggregator> NewAggregator() const override;
 
-  bool HasValue(const DataFile& file) const override;
-
  private:
   explicit MaxAggregate(std::shared_ptr<BoundTerm> term);
 };
@@ -229,12 +225,11 @@ class ICEBERG_EXPORT MinAggregate : public BoundAggregate {
   static std::shared_ptr<MinAggregate> Make(std::shared_ptr<BoundTerm> term);
 
   Result<Literal> Evaluate(const StructLike& data) const override;
-  Result<Literal> Evaluate(const DataFile& file) const final;
+  Result<Literal> Evaluate(const DataFile& file) const override;
+  bool HasValue(const DataFile& file) const override;
 
   std::unique_ptr<Aggregator> NewAggregator() const override;
 
-  bool HasValue(const DataFile& file) const override;
-
  private:
   explicit MinAggregate(std::shared_ptr<BoundTerm> term);
 };

src/iceberg/expression/term.h

@@ -37,10 +37,13 @@ namespace iceberg {
 /// \brief A term is an expression node that produces a typed value when evaluated.
 class ICEBERG_EXPORT Term : public util::Formattable {
  public:
-  enum class Kind : uint8_t { kReference = 0, kTransform, kExtract };
+  enum class Kind : uint8_t { kReference, kTransform, kExtract };
 
   /// \brief Returns the kind of this term.
   virtual Kind kind() const = 0;
+
+  /// \brief Returns whether this term is unbound.
+  virtual bool is_unbound() const = 0;
 };
 
 template <typename T>
@@ -53,6 +56,8 @@ template <typename B>
 class ICEBERG_EXPORT UnboundTerm : public Unbound<B>, public Term {
  public:
   using BoundType = B;
+
+  bool is_unbound() const override { return true; }
 };
 
 /// \brief Base class for bound terms.
@@ -66,8 +71,6 @@ class ICEBERG_EXPORT BoundTerm : public Bound, public Term {
   /// \brief Returns whether this term may produce null values.
   virtual bool MayProduceNull() const = 0;
 
-  // TODO(gangwu): add a comparator function to Literal and BoundTerm.
-
   /// \brief Returns whether this term is equivalent to another.
   ///
   /// Two terms are equivalent if they produce the same values when evaluated.
@@ -79,6 +82,8 @@ class ICEBERG_EXPORT BoundTerm : public Bound, public Term {
   friend bool operator==(const BoundTerm& lhs, const BoundTerm& rhs) {
     return lhs.Equals(rhs);
   }
+
+  bool is_unbound() const override { return false; }
 };
 
 /// \brief A reference represents a named field in an expression.
@@ -114,6 +119,8 @@ class ICEBERG_EXPORT NamedReference
 
   Kind kind() const override { return Kind::kReference; }
 
+  bool is_unbound() const override { return true; }
+
  private:
   explicit NamedReference(std::string field_name);
 
@@ -152,6 +159,8 @@ class ICEBERG_EXPORT BoundReference
 
   Kind kind() const override { return Kind::kReference; }
 
+  bool is_unbound() const override { return false; }
+
  private:
   BoundReference(SchemaField field, std::unique_ptr<StructLikeAccessor> accessor);
 

src/iceberg/row/struct_like.cc

@@ -68,15 +68,6 @@ Result<Scalar> LiteralToScalar(const Literal& literal) {
   }
 }
 
-SingleValueStructLike::SingleValueStructLike(Literal literal)
-    : literal_(std::move(literal)) {}
-
-Result<Scalar> SingleValueStructLike::GetField(size_t /*pos*/) const {
-  return LiteralToScalar(literal_);
-}
-
-size_t SingleValueStructLike::num_fields() const { return 1; }
-
 StructLikeAccessor::StructLikeAccessor(std::shared_ptr<Type> type,
                                        std::span<const size_t> position_path)
     : type_(std::move(type)) {

src/iceberg/row/struct_like.h

@@ -71,19 +71,6 @@ class ICEBERG_EXPORT StructLike {
   virtual size_t num_fields() const = 0;
 };
 
-/// \brief A single-field StructLike that wraps a Literal
-class ICEBERG_EXPORT SingleValueStructLike : public StructLike {
- public:
-  explicit SingleValueStructLike(Literal literal);
-
-  Result<Scalar> GetField(size_t pos) const override;
-
-  size_t num_fields() const override;
-
- private:
-  Literal literal_;
-};
-
 /// \brief An immutable array-like wrapper.
 class ICEBERG_EXPORT ArrayLike {
  public:

src/iceberg/test/aggregate_test.cc

@@ -400,7 +400,7 @@ TEST(AggregateTest, DataFileAggregatorParity) {
     for (const auto& f : files) {
       ASSERT_TRUE(evaluator->Update(f).has_value());
     }
-    EXPECT_EQ(evaluator->AllAggregatorsValid(), expect_all_valid);
+    ASSERT_EQ(evaluator->AllAggregatorsValid(), expect_all_valid);
     ICEBERG_UNWRAP_OR_FAIL(auto results, evaluator->GetResults());
     ASSERT_EQ(results.size(), expected.size());
     for (size_t i = 0; i < expected.size(); ++i) {