simplify a little bit

Author: wgtmac

src/iceberg/expression/residual_evaluator.cc

@@ -19,7 +19,6 @@
 
 #include "iceberg/expression/residual_evaluator.h"
 
-#include "iceberg/expression/binder.h"
 #include "iceberg/expression/expression.h"
 #include "iceberg/expression/expression_visitor.h"
 #include "iceberg/expression/predicate.h"
@@ -187,19 +186,19 @@ class ResidualVisitor : public BoundVisitor<std::shared_ptr<Expression>> {
 
   Result<std::shared_ptr<Expression>> Predicate(
       const std::shared_ptr<UnboundPredicate>& pred) override {
-    ICEBERG_ASSIGN_OR_RAISE(auto bound_predicate, pred->Bind(schema_, case_sensitive_));
-    if (bound_predicate->is_bound_predicate()) {
+    ICEBERG_ASSIGN_OR_RAISE(auto bound, pred->Bind(schema_, case_sensitive_));
+    if (bound->is_bound_predicate()) {
       ICEBERG_ASSIGN_OR_RAISE(
           auto residual,
-          Predicate(std::dynamic_pointer_cast<BoundPredicate>(bound_predicate)));
+          Predicate(internal::checked_pointer_cast<BoundPredicate>(bound)));
       if (residual->is_bound_predicate()) {
         // replace inclusive original unbound predicate
         return pred;
       }
       return residual;
     }
     // if binding didn't result in a Predicate, return the expression
-    return bound_predicate;
+    return bound;
   }
 
  private:
@@ -218,7 +217,6 @@ class ResidualVisitor : public BoundVisitor<std::shared_ptr<Expression>> {
   const StructLike& partition_data_;
   bool case_sensitive_;
 };
-}  // namespace
 
 Result<std::shared_ptr<Expression>> ResidualVisitor::Predicate(
     const std::shared_ptr<BoundPredicate>& pred) {
@@ -230,37 +228,30 @@ Result<std::shared_ptr<Expression>> ResidualVisitor::Predicate(
   // have returned false, so the predicate can also be eliminated if the inclusive
   // projection evaluates to false.
 
-  // Get the field ID from the predicate's reference
-  const auto& ref = pred->reference();
-  int32_t field_id = ref->field().field_id();
-
-  // Find partition fields that match this source field ID
-  auto matching_fields = spec_.GetFieldsBySourceId(field_id);
-
-  if (!matching_fields.has_value()) {
+  // If there is no strict projection or if it evaluates to false, then return the
+  // predicate.
+  ICEBERG_ASSIGN_OR_RAISE(
+      auto parts, spec_.GetFieldsBySourceId(pred->reference()->field().field_id()));
+  if (parts.empty()) {
     // Not associated with a partition field, can't be evaluated
     return pred;
   }
 
-  for (const auto& part : matching_fields.value()) {
+  for (const auto& part : parts) {
     // Check the strict projection
     ICEBERG_ASSIGN_OR_RAISE(auto strict_projection, part.get().transform()->ProjectStrict(
                                                         part.get().name(), pred));
     std::shared_ptr<Expression> strict_result = nullptr;
 
     if (strict_projection != nullptr) {
-      // Bind the projected predicate to partition type
       ICEBERG_ASSIGN_OR_RAISE(
           auto bound_strict,
-          Binder::Bind(*partition_schema_,
-                       std::shared_ptr<Expression>(std::move(strict_projection)),
-                       case_sensitive_));
-
+          strict_projection->Bind(*partition_schema_, case_sensitive_));
       if (bound_strict->is_bound_predicate()) {
-        // Evaluate the bound predicate against partition data
         ICEBERG_ASSIGN_OR_RAISE(
-            strict_result, BoundVisitor::Predicate(
-                               std::dynamic_pointer_cast<BoundPredicate>(bound_strict)));
+            strict_result,
+            BoundVisitor::Predicate(
+                internal::checked_pointer_cast<BoundPredicate>(bound_strict)));
       } else {
         // If the result is not a predicate, then it must be a constant like alwaysTrue
         // or alwaysFalse
@@ -279,19 +270,15 @@ Result<std::shared_ptr<Expression>> ResidualVisitor::Predicate(
     std::shared_ptr<Expression> inclusive_result = nullptr;
 
     if (inclusive_projection != nullptr) {
-      // Bind the projected predicate to partition type
       ICEBERG_ASSIGN_OR_RAISE(
           auto bound_inclusive,
-          Binder::Bind(*partition_schema_,
-                       std::shared_ptr<Expression>(std::move(inclusive_projection)),
-                       case_sensitive_));
+          inclusive_projection->Bind(*partition_schema_, case_sensitive_));
 
       if (bound_inclusive->is_bound_predicate()) {
-        // Evaluate the bound predicate against partition data
         ICEBERG_ASSIGN_OR_RAISE(
             inclusive_result,
             BoundVisitor::Predicate(
-                std::dynamic_pointer_cast<BoundPredicate>(bound_inclusive)));
+                internal::checked_pointer_cast<BoundPredicate>(bound_inclusive)));
       } else {
         // If the result is not a predicate, then it must be a constant like alwaysTrue
         // or alwaysFalse
@@ -310,24 +297,12 @@ Result<std::shared_ptr<Expression>> ResidualVisitor::Predicate(
   return pred;
 }
 
-ResidualEvaluator::ResidualEvaluator(std::shared_ptr<Expression> expr,
-                                     const PartitionSpec& spec, const Schema& schema,
-                                     bool case_sensitive)
-    : expr_(std::move(expr)),
-      spec_(spec),
-      schema_(schema),
-      case_sensitive_(case_sensitive) {}
-
-ResidualEvaluator::~ResidualEvaluator() = default;
-
-namespace {
-
 // Unpartitioned residual evaluator that always returns the original expression
 class UnpartitionedResidualEvaluator : public ResidualEvaluator {
  public:
   explicit UnpartitionedResidualEvaluator(std::shared_ptr<Expression> expr)
       : ResidualEvaluator(std::move(expr), *PartitionSpec::Unpartitioned(),
-                          *empty_schema_, true) {}
+                          *kEmptySchema_, true) {}
 
   Result<std::shared_ptr<Expression>> ResidualFor(
       const StructLike& /*partition_data*/) const override {
@@ -336,15 +311,22 @@ class UnpartitionedResidualEvaluator : public ResidualEvaluator {
 
  private:
   // Store an empty schema to avoid dangling reference when passing to base class
-  static const std::shared_ptr<Schema> empty_schema_;
+  inline static const std::shared_ptr<Schema> kEmptySchema_ =
+      std::make_shared<Schema>(std::vector<SchemaField>{}, std::nullopt);
 };
 
-// Static member definition
-const std::shared_ptr<Schema> UnpartitionedResidualEvaluator::empty_schema_ =
-    std::make_shared<Schema>(std::vector<SchemaField>{}, std::nullopt);
-
 }  // namespace
 
+ResidualEvaluator::ResidualEvaluator(std::shared_ptr<Expression> expr,
+                                     const PartitionSpec& spec, const Schema& schema,
+                                     bool case_sensitive)
+    : expr_(std::move(expr)),
+      spec_(spec),
+      schema_(schema),
+      case_sensitive_(case_sensitive) {}
+
+ResidualEvaluator::~ResidualEvaluator() = default;
+
 Result<std::unique_ptr<ResidualEvaluator>> ResidualEvaluator::Unpartitioned(
     std::shared_ptr<Expression> expr) {
   return std::unique_ptr<ResidualEvaluator>(

src/iceberg/partition_spec.cc

@@ -162,7 +162,8 @@ PartitionSpec::GetFieldsBySourceId(int32_t source_id) const {
       it != source_id_to_fields.get().cend()) {
     return it->second;
   }
-  return NotFound("Cannot find partition fields for source id: {}", source_id);
+  // Note that it is not an error to not find any partition fields for a source id.
+  return std::vector<PartitionFieldRef>{};
 }
 
 Result<PartitionSpec::SourceIdToFieldsMap> PartitionSpec::InitSourceIdToFieldsMap(

src/iceberg/partition_spec.h

@@ -62,7 +62,7 @@ class ICEBERG_EXPORT PartitionSpec : public util::Formattable {
   std::span<const PartitionField> fields() const;
 
   /// \brief Get the partition type binding to the input schema.
-  Result<std::unique_ptr<StructType>> PartitionType(const Schema&) const;
+  Result<std::unique_ptr<StructType>> PartitionType(const Schema& schema) const;
 
   std::string ToString() const override;
 
@@ -83,8 +83,8 @@ class ICEBERG_EXPORT PartitionSpec : public util::Formattable {
   /// \param source_id The id of the source field.
   /// \return The partition fields by source ID, or NotFound if the source field is not
   /// found.
-  Result<std::vector<std::reference_wrapper<const PartitionField>>> GetFieldsBySourceId(
-      int32_t source_id) const;
+  using PartitionFieldRef = std::reference_wrapper<const PartitionField>;
+  Result<std::vector<PartitionFieldRef>> GetFieldsBySourceId(int32_t source_id) const;
 
   /// \brief Create a PartitionSpec binding to a schema.
   /// \param schema The schema to bind the partition spec to.
@@ -125,9 +125,7 @@ class ICEBERG_EXPORT PartitionSpec : public util::Formattable {
   /// \brief Compare two partition specs for equality.
   bool Equals(const PartitionSpec& other) const;
 
-  using SourceIdToFieldsMap =
-      std::unordered_map<int32_t,
-                         std::vector<std::reference_wrapper<const PartitionField>>>;
+  using SourceIdToFieldsMap = std::unordered_map<int32_t, std::vector<PartitionFieldRef>>;
   static Result<SourceIdToFieldsMap> InitSourceIdToFieldsMap(const PartitionSpec&);
 
   const int32_t spec_id_;