fix: review comments and refactor transform test

Author: zhjwpku

src/iceberg/expression/literal.cc

@@ -27,7 +27,6 @@
 #include "iceberg/exception.h"
 #include "iceberg/util/checked_cast.h"
 #include "iceberg/util/conversions.h"
-#include "iceberg/util/decimal.h"
 #include "iceberg/util/macros.h"
 
 namespace iceberg {
@@ -190,11 +189,14 @@ Result<Literal> LiteralCaster::CastFromString(
   const auto& str_val = std::get<std::string>(literal.value_);
 
   switch (target_type->type_id()) {
+    case TypeId::kUuid: {
+      ICEBERG_ASSIGN_OR_RAISE(auto uuid, Uuid::FromString(str_val));
+      return Literal::UUID(uuid);
+    }
     case TypeId::kDate:
     case TypeId::kTime:
     case TypeId::kTimestamp:
     case TypeId::kTimestampTz:
-    case TypeId::kUuid:
       return NotImplemented("Cast from String to {} is not implemented yet",
                             target_type->ToString());
     default:
@@ -337,31 +339,39 @@ std::strong_ordering CompareFloat(T lhs, T rhs) {
   return lhs_is_negative <=> rhs_is_negative;
 }
 
-std::strong_ordering CompareDecimal(Literal const& lhs, Literal const& rhs) {
+std::partial_ordering CompareDecimal(Literal const& lhs, Literal const& rhs) {
   ICEBERG_DCHECK(std::holds_alternative<Decimal>(lhs.value()),
                  "LHS of decimal comparison must hold Decimal");
   ICEBERG_DCHECK(std::holds_alternative<Decimal>(rhs.value()),
                  "RHS of decimal comparison must hold decimal");
-  const auto& lhs_type = std::dynamic_pointer_cast<DecimalType>(lhs.type());
-  const auto& rhs_type = std::dynamic_pointer_cast<DecimalType>(rhs.type());
+  auto lhs_type = internal::checked_pointer_cast<DecimalType>(lhs.type());
+  auto rhs_type = internal::checked_pointer_cast<DecimalType>(rhs.type());
   auto lhs_decimal = std::get<Decimal>(lhs.value());
   auto rhs_decimal = std::get<Decimal>(rhs.value());
   if (lhs_type->scale() == rhs_type->scale()) {
     return lhs_decimal <=> rhs_decimal;
   } else if (lhs_type->scale() > rhs_type->scale()) {
     // Rescale to larger scale
     auto rhs_res = rhs_decimal.Rescale(rhs_type->scale(), lhs_type->scale());
-    if (!rhs_res) {
-      // Rescale would cause data loss, so lhs is definitely less than rhs
-      return std::strong_ordering::less;
+    if (!rhs_res.has_value()) {
+      if (rhs_res.error().kind == ErrorKind::kRescaleDataLoss) {
+        // Rescale would cause data loss, so lhs is definitely less than rhs
+        return std::partial_ordering::less;
+      }
+      // Other errors should return unordered
+      return std::partial_ordering::unordered;
     }
     return lhs_decimal <=> rhs_res.value();
   } else {
     // Rescale to larger scale
     auto lhs_res = lhs_decimal.Rescale(lhs_type->scale(), rhs_type->scale());
-    if (!lhs_res) {
-      // Rescale would cause data loss, so lhs is definitely greater than rhs
-      return std::strong_ordering::greater;
+    if (!lhs_res.has_value()) {
+      if (lhs_res.error().kind == ErrorKind::kRescaleDataLoss) {
+        // Rescale would cause data loss, so lhs is definitely greater than rhs
+        return std::partial_ordering::greater;
+      }
+      // Other errors should return unordered
+      return std::partial_ordering::unordered;
     }
     return lhs_res.value() <=> rhs_decimal;
   }

src/iceberg/result.h

@@ -47,6 +47,7 @@ enum class ErrorKind {
   kNotFound,
   kNotImplemented,
   kNotSupported,
+  kRescaleDataLoss,
   kUnknownError,
 };
 
@@ -95,6 +96,7 @@ DEFINE_ERROR_FUNCTION(NotAllowed)
 DEFINE_ERROR_FUNCTION(NotFound)
 DEFINE_ERROR_FUNCTION(NotImplemented)
 DEFINE_ERROR_FUNCTION(NotSupported)
+DEFINE_ERROR_FUNCTION(RescaleDataLoss)
 DEFINE_ERROR_FUNCTION(UnknownError)
 
 #undef DEFINE_ERROR_FUNCTION

src/iceberg/test/decimal_test.cc

@@ -712,7 +712,7 @@ TEST(DecimalTest, Rescale) {
   ASSERT_EQ(Decimal(5), Decimal(500000).Rescale(6, 1).value());
   ASSERT_EQ(Decimal(500000), Decimal(5).Rescale(1, 6).value());
 
-  ASSERT_THAT(Decimal(5555555).Rescale(6, 1), IsError(ErrorKind::kInvalid));
+  ASSERT_THAT(Decimal(5555555).Rescale(6, 1), IsError(ErrorKind::kRescaleDataLoss));
 }
 
 }  // namespace iceberg

src/iceberg/test/literal_test.cc

@@ -719,6 +719,13 @@ INSTANTIATE_TEST_SUITE_P(
                              .target_type = fixed(4),
                              .expected_literal = Literal::Fixed(std::vector<uint8_t>{
                                  0x01, 0x02, 0x03, 0x04})},
+        // String cast tests
+        CastLiteralTestParam{
+            .test_name = "StringToUuid",
+            .source_literal = Literal::String("123e4567-e89b-12d3-a456-426614174000"),
+            .target_type = uuid(),
+            .expected_literal = Literal::UUID(
+                Uuid::FromString("123e4567-e89b-12d3-a456-426614174000").value())},
         // Same type cast test
         CastLiteralTestParam{.test_name = "IntToInt",
                              .source_literal = Literal::Int(42),