fix review comments

Author: zhjwpku

src/iceberg/sort_order.cc

@@ -41,7 +41,7 @@ std::span<const SortField> SortOrder::fields() const { return fields_; }
 
 bool SortOrder::Satisfies(const SortOrder& other) const {
   // any ordering satisfies an unsorted ordering
-  if (other.IsUnsorted()) {
+  if (other.is_unsorted()) {
     return true;
   }
 
@@ -61,17 +61,7 @@ bool SortOrder::Satisfies(const SortOrder& other) const {
 }
 
 bool SortOrder::SameOrder(const SortOrder& other) const {
-  if (fields_.size() != other.fields().size()) {
-    return false;
-  }
-
-  for (const auto& [field, other_field] : std::views::zip(fields_, other.fields())) {
-    if (!(field == other_field)) {
-      return false;
-    }
-  }
-
-  return true;
+  return fields_ == other.fields_;
 }
 
 std::string SortOrder::ToString() const {

src/iceberg/sort_order.h

@@ -50,11 +50,11 @@ class ICEBERG_EXPORT SortOrder : public util::Formattable {
   std::span<const SortField> fields() const;
 
   /// \brief Returns true if the sort order is sorted
-  bool IsSorted() const { return !fields_.empty(); }
+  bool is_sorted() const { return !fields_.empty(); }
 
   /// \brief Returns true if the sort order is unsorted
   /// A SortOrder is unsorted if it has no sort fields.
-  bool IsUnsorted() const { return fields_.empty(); }
+  bool is_unsorted() const { return fields_.empty(); }
 
   /// \brief Checks whether this order satisfies another order.
   bool Satisfies(const SortOrder& other) const;

src/iceberg/test/sort_order_test.cc

@@ -84,21 +84,21 @@ TEST(SortOrderTest, Equality) {
   ASSERT_NE(sort_order5, sort_order1);
 }
 
-TEST(SortOrderTest, IsUnsorted) {
+TEST(SortOrderTest, is_unsorted) {
   auto unsorted = SortOrder::Unsorted();
-  EXPECT_TRUE(unsorted->IsUnsorted());
-  EXPECT_FALSE(unsorted->IsSorted());
+  EXPECT_TRUE(unsorted->is_unsorted());
+  EXPECT_FALSE(unsorted->is_sorted());
 }
 
-TEST(SortOrderTest, IsSorted) {
+TEST(SortOrderTest, is_sorted) {
   SchemaField field1(5, "ts", iceberg::timestamp(), true);
   auto identity_transform = Transform::Identity();
   SortField st_field1(5, identity_transform, SortDirection::kAscending,
                       NullOrder::kFirst);
   SortOrder sorted_order(100, {st_field1});
 
-  EXPECT_TRUE(sorted_order.IsSorted());
-  EXPECT_FALSE(sorted_order.IsUnsorted());
+  EXPECT_TRUE(sorted_order.is_sorted());
+  EXPECT_FALSE(sorted_order.is_unsorted());
 }
 
 TEST(SortOrderTest, Satisfies) {

src/iceberg/transform.cc

@@ -21,6 +21,7 @@
 
 #include <format>
 #include <regex>
+#include <utility>
 
 #include "iceberg/transform_function.h"
 #include "iceberg/type.h"
@@ -130,16 +131,29 @@ bool Transform::PreservesOrder() const {
     case TransformType::kVoid:
     case TransformType::kBucket:
       return false;
-    default:
+    case TransformType::kIdentity:
+    case TransformType::kTruncate:
+    case TransformType::kYear:
+    case TransformType::kMonth:
+    case TransformType::kDay:
+    case TransformType::kHour:
       return true;
   }
+  std::unreachable();
 }
 
 bool Transform::SatisfiesOrderOf(const Transform& other) const {
   auto other_type = other.transform_type();
   switch (transform_type_) {
     case TransformType::kIdentity:
+      // ordering by value is the same as long as the other preserves order
       return other.PreservesOrder();
+    case TransformType::kTruncate: {
+      if (other_type != TransformType::kTruncate) {
+        return false;
+      }
+      return std::get<int32_t>(param_) >= std::get<int32_t>(other.param_);
+    }
     case TransformType::kHour:
       return other_type == TransformType::kHour || other_type == TransformType::kDay ||
              other_type == TransformType::kMonth || other_type == TransformType::kYear;
@@ -148,9 +162,13 @@ bool Transform::SatisfiesOrderOf(const Transform& other) const {
              other_type == TransformType::kYear;
     case TransformType::kMonth:
       return other_type == TransformType::kMonth || other_type == TransformType::kYear;
-    default:
+    case TransformType::kYear:
+    case TransformType::kBucket:
+    case TransformType::kUnknown:
+    case TransformType::kVoid:
       return *this == other;
   }
+  std::unreachable();
 }
 
 bool TransformFunction::Equals(const TransformFunction& other) const {