review

Author: Innocent

src/iceberg/expression/json_serde.cc

@@ -35,8 +35,8 @@ namespace iceberg {
 Result<std::shared_ptr<Expression>> ExpressionFromJson(const nlohmann::json& json) {
   // Handle boolean
   if (json.is_boolean()) {
-    return json.get<bool>() ? std::static_pointer_cast<Expression>(True::Instance())
-                            : std::static_pointer_cast<Expression>(False::Instance());
+    return json.get<bool>() ? std::dynamic_pointer_cast<Expression>(True::Instance())
+                            : std::dynamic_pointer_cast<Expression>(False::Instance());
   }
   return JsonParseError("Only boolean are currently supported");
 }

src/iceberg/expression/json_serde_internal.h

@@ -25,7 +25,7 @@
 #include "iceberg/iceberg_export.h"
 #include "iceberg/result.h"
 
-/// \file iceberg/expression/json_internal.h
+/// \file iceberg/expression/json_serde_internal.h
 /// JSON serialization and deserialization for expressions.
 
 namespace iceberg {
@@ -43,7 +43,7 @@ Result<Model> FromJson(const nlohmann::json& json);
   ICEBERG_EXPORT nlohmann::json ToJson(const Model& model);
 
 /// \note Don't forget to add `ICEBERG_DEFINE_FROM_JSON` to the end of
-/// `json_internal.cc` to define the `FromJson` function for the model.
+/// `json_serde_internal.cc` to define the `FromJson` function for the model.
 ICEBERG_DECLARE_JSON_SERDE(Expression)
 
 #undef ICEBERG_DECLARE_JSON_SERDE

src/iceberg/expression/meson.build

@@ -24,6 +24,7 @@ install_headers(
         'expression_visitor.h',
         'expressions.h',
         'inclusive_metrics_evaluator.h',
+        'json_serde_internal.h',
         'literal.h',
         'manifest_evaluator.h',
         'predicate.h',

src/iceberg/test/expression_json_test.cc

@@ -34,35 +34,27 @@
 
 namespace iceberg {
 
-class ExpressionJsonTest : public ::testing::Test {};
+class ExpressionJsonTest : public ::testing::Test {
+ protected:
+  void CheckBoolean(std::shared_ptr<Expression> expr, bool value) {
+    auto json = ToJson(*expr);
+    EXPECT_TRUE(json.is_boolean());
+    EXPECT_EQ(json.get<bool>(), value);
+
+    auto result = ExpressionFromJson(json);
+    ASSERT_THAT(result, IsOk());
+    if (value) {
+      EXPECT_EQ(result.value()->op(), Expression::Operation::kTrue);
+    } else {
+      EXPECT_EQ(result.value()->op(), Expression::Operation::kFalse);
+    }
+  }
+};
 
 // Test boolean constant expressions
-TEST_F(ExpressionJsonTest, TrueExpression) {
-  auto expr = True::Instance();
-  auto json = ToJson(*expr);
-
-  // True should serialize as JSON boolean true
-  EXPECT_TRUE(json.is_boolean());
-  EXPECT_TRUE(json.get<bool>());
-
-  // Parse back
-  auto result = ExpressionFromJson(json);
-  ASSERT_THAT(result, IsOk());
-  EXPECT_EQ(result.value()->op(), Expression::Operation::kTrue);
-}
-
-TEST_F(ExpressionJsonTest, FalseExpression) {
-  auto expr = False::Instance();
-  auto json = ToJson(*expr);
-
-  // False should serialize as JSON boolean false
-  EXPECT_TRUE(json.is_boolean());
-  EXPECT_FALSE(json.get<bool>());
-
-  // Parse back
-  auto result = ExpressionFromJson(json);
-  ASSERT_THAT(result, IsOk());
-  EXPECT_EQ(result.value()->op(), Expression::Operation::kFalse);
+TEST_F(ExpressionJsonTest, CheckBooleanExpression) {
+  CheckBoolean(True::Instance(), true);
+  CheckBoolean(False::Instance(), false);
 }
 
 }  // namespace iceberg

src/iceberg/test/meson.build

@@ -61,6 +61,7 @@ iceberg_tests = {
     'expression_test': {
         'sources': files(
             'aggregate_test.cc',
+            'expression_json_test.cc',
             'expression_test.cc',
             'expression_visitor_test.cc',
             'inclusive_metrics_evaluator_test.cc',