From dd93910ee2418c85aaedacf72eeef51c435d4147 Mon Sep 17 00:00:00 2001
From: Christian Thiel <christian@hansetag.com>
Date: Sun, 8 Sep 2024 11:21:05 +0200
Subject: [PATCH 1/3] SortOrder methods should take schema ref if possible

---
 crates/iceberg/src/spec/sort.rs | 49 +++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/crates/iceberg/src/spec/sort.rs b/crates/iceberg/src/spec/sort.rs
index 82909344a0..8de882aedb 100644
--- a/crates/iceberg/src/spec/sort.rs
+++ b/crates/iceberg/src/spec/sort.rs
@@ -133,6 +133,14 @@ impl SortOrder {
     pub fn is_unsorted(&self) -> bool {
         self.fields.is_empty()
     }
+
+    /// Set the order id for the sort order
+    pub fn with_order_id(&self, order_id: i64) -> SortOrder {
+        SortOrder {
+            order_id,
+            fields: self.fields.clone(),
+        }
+    }
 }
 
 impl SortOrderBuilder {
@@ -160,13 +168,13 @@ impl SortOrderBuilder {
     }
 
     /// Creates a new bound sort order.
-    pub fn build(&self, schema: Schema) -> Result<SortOrder> {
+    pub fn build(&self, schema: &Schema) -> Result<SortOrder> {
         let unbound_sort_order = self.build_unbound()?;
         SortOrderBuilder::check_compatibility(unbound_sort_order, schema)
     }
 
     /// Returns the given sort order if it is compatible with the given schema
-    fn check_compatibility(sort_order: SortOrder, schema: Schema) -> Result<SortOrder> {
+    fn check_compatibility(sort_order: SortOrder, schema: &Schema) -> Result<SortOrder> {
         let sort_fields = &sort_order.fields;
         for sort_field in sort_fields {
             match schema.field_by_id(sort_field.source_id) {
@@ -290,6 +298,35 @@ mod tests {
         )
     }
 
+    #[test]
+    fn test_build_unbound_returns_correct_default_order_id_for_no_fields() {
+        assert_eq!(
+            SortOrder::builder()
+                .build_unbound()
+                .expect("Expected an Ok value")
+                .order_id,
+            SortOrder::UNSORTED_ORDER_ID
+        )
+    }
+
+    #[test]
+    fn test_build_unbound_returns_correct_default_order_id_for_fields() {
+        let sort_field = SortField::builder()
+            .source_id(2)
+            .direction(SortDirection::Ascending)
+            .null_order(NullOrder::First)
+            .transform(Transform::Identity)
+            .build();
+        assert_ne!(
+            SortOrder::builder()
+                .with_sort_field(sort_field.clone())
+                .build_unbound()
+                .expect("Expected an Ok value")
+                .order_id,
+            SortOrder::UNSORTED_ORDER_ID
+        )
+    }
+
     #[test]
     fn test_build_unbound_should_return_unsorted_sort_order() {
         assert_eq!(
@@ -367,7 +404,7 @@ mod tests {
                     .transform(Transform::Identity)
                     .build(),
             )
-            .build(schema);
+            .build(&schema);
 
         assert_eq!(
             sort_order_builder_result
@@ -406,7 +443,7 @@ mod tests {
                     .transform(Transform::Identity)
                     .build(),
             )
-            .build(schema);
+            .build(&schema);
 
         assert_eq!(
             sort_order_builder_result
@@ -438,7 +475,7 @@ mod tests {
                     .transform(Transform::Year)
                     .build(),
             )
-            .build(schema);
+            .build(&schema);
 
         assert_eq!(
             sort_order_builder_result
@@ -468,7 +505,7 @@ mod tests {
 
         let sort_order_builder_result = SortOrder::builder()
             .with_sort_field(sort_field.clone())
-            .build(schema);
+            .build(&schema);
 
         assert_eq!(
             sort_order_builder_result.expect("Expected an Ok value"),

From 148a4e6401fafe2e0c4e80a781cab8659ec843ea Mon Sep 17 00:00:00 2001
From: Christian Thiel <christian@hansetag.com>
Date: Sun, 8 Sep 2024 11:29:22 +0200
Subject: [PATCH 2/3] Fix test type

---
 crates/catalog/memory/src/catalog.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/catalog/memory/src/catalog.rs b/crates/catalog/memory/src/catalog.rs
index 05038cb663..1da044821e 100644
--- a/crates/catalog/memory/src/catalog.rs
+++ b/crates/catalog/memory/src/catalog.rs
@@ -371,7 +371,7 @@ mod tests {
         let expected_sorted_order = SortOrder::builder()
             .with_order_id(0)
             .with_fields(vec![])
-            .build(expected_schema.clone())
+            .build(expected_schema)
             .unwrap();
 
         assert_eq!(

From 67cf1400de7acd66d21eb9cf2a63b057308f6e74 Mon Sep 17 00:00:00 2001
From: Christian Thiel <christian@hansetag.com>
Date: Sun, 8 Sep 2024 17:47:19 +0200
Subject: [PATCH 3/3] with_order_id should not take reference

---
 crates/iceberg/src/spec/sort.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/iceberg/src/spec/sort.rs b/crates/iceberg/src/spec/sort.rs
index 8de882aedb..5e50a175c9 100644
--- a/crates/iceberg/src/spec/sort.rs
+++ b/crates/iceberg/src/spec/sort.rs
@@ -135,10 +135,10 @@ impl SortOrder {
     }
 
     /// Set the order id for the sort order
-    pub fn with_order_id(&self, order_id: i64) -> SortOrder {
+    pub fn with_order_id(self, order_id: i64) -> SortOrder {
         SortOrder {
             order_id,
-            fields: self.fields.clone(),
+            fields: self.fields,
         }
     }
 }