feat: add json serde for expressions by evindj · Pull Request #553 · apache/iceberg-cpp

evindj · 2026-02-02T16:05:59Z

This is the second PR in addressing serde for expression for predicate push down. It focuses on unbound transforms and and named references.
Added unit tests to ensure that serialization happens correctly. while name references will map just names like schema columns, unbound transforms will make sure take care of transform serde. for example it will ensure

{ "type": "transform", "transform": "bucket[16]", "term": "id" }

is converted to an object and vice versa.

zhjwpku · 2026-02-03T06:23:06Z

src/iceberg/expression/json_serde.cc

+
+Result<std::shared_ptr<NamedReference>> NamedReferenceFromJson(
+    const nlohmann::json& json) {
+  if (!json.is_string()) {


add a [[unlikely]] hint?

zhjwpku · 2026-02-03T06:27:06Z

src/iceberg/expression/json_serde.cc

 * under the License.
 */

+#include <cctype>


do we need this header?

zhjwpku · 2026-02-03T06:30:01Z

src/iceberg/expression/json_serde.cc

+nlohmann::json ToJson(const UnboundTransform& transform) {
+  auto& mutable_transform = const_cast<UnboundTransform&>(transform);
+  nlohmann::json json;
+  json[kType] = std::string(kTransform);


Suggested change

json[kType] = std::string(kTransform);

json[kType] = kTransform;

I don't think the string ctor around kTransform is necessary. The same applies to other places as well.

windows does not seem to like it when we don't do the explicit copy.

Oh, I'm not entirely sure, but I noticed a similar usage here: https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/json_serde.cc#L1295

What is the error message in the failed windows ci? Can we figure it out how to fix this since other places are good with it?

zhjwpku · 2026-02-04T10:48:24Z

src/iceberg/expression/json_serde.cc

+
+Result<std::shared_ptr<UnboundTransform>> UnboundTransformFromJson(
+    const nlohmann::json& json) {
+  if (json.is_object() && json.contains(kType) && json[kType] == kTransform &&


From the build error msg, it seems windows don't like json[kType] == kTransform, you might change this back.

zhjwpku · 2026-02-04T10:49:28Z

src/iceberg/expression/json_serde.cc

+  nlohmann::json json;
+  json[kType] = kTransform;
+  json[kTransform] = transform.transform()->ToString();
+  json[kTerm] = std::string(mutable_transform.reference()->name());


Suggested change

json[kTerm] = std::string(mutable_transform.reference()->name());

json[kTerm] = mutable_transform.reference()->name();

line 146 seems ok, so the same apply to line 148?

I donl think like 146 is ok, after some research, I think what is happening here is the following: MSVC is having hard times deciding what overload for operators to use between
nlohmann::json::operator== (from nlohmann_json library)
std::operator==<char, std::char_traits> for string_view (from MSVC's STL)

a similar confusion is happening for constructors. I can push a another diff to confirm, I will push another version to confirm

Good to know. It seems that Compiler Explorer doesn't currently support libraries with MSVC[1], so we have to rely on the CI to confirm that.

[1] https://godbolt.org/z/4nsKx3o5o

wgtmac · 2026-02-06T09:21:31Z

src/iceberg/type_fwd.h

 class Literal;
 class Term;
 class UnboundPredicate;
+class NamedReference;


Let's sort them alphabetically.

wgtmac · 2026-02-09T06:32:49Z

src/iceberg/test/expression_json_test.cc

  EXPECT_FALSE(IsUnaryOperation(Expression::Operation::kTrue));
 }

+TEST(ExpressionJsonTest, NameReferenceRoundTrip) {


Can we try to reuse TestJsonConversion to avoid boilerplate of round trip test like this?

iceberg-cpp/src/iceberg/test/json_serde_test.cc

Lines 76 to 84 in b15ac65

void TestJsonConversion(const T& obj, const nlohmann::json& expected_json) {

auto json = ToJson(obj);

EXPECT_EQ(expected_json, json) << "JSON conversion mismatch.";

// Specialize FromJson based on type (T)

auto obj_ex = FromJsonHelper<T>(expected_json);

EXPECT_TRUE(obj_ex.has_value()) << "Failed to deserialize JSON.";

EXPECT_EQ(obj, *obj_ex.value()) << "Deserialized object mismatch.";

}

? We can use a simple for-loop or even parameterized test pattern so we don't need to repeat this when adding new expr serde later.

wgtmac · 2026-02-09T06:36:41Z

src/iceberg/expression/json_serde.cc

+nlohmann::json ToJson(const UnboundTransform& transform) {
+  auto& mutable_transform = const_cast<UnboundTransform&>(transform);
+  nlohmann::json json;
+  json[kType] = std::string(kTransform);


What is the error message in the failed windows ci? Can we figure it out how to fix this since other places are good with it?

wgtmac · 2026-02-09T06:47:37Z

src/iceberg/expression/json_serde.cc

+  ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReference::Make(json.get<std::string>()));
+  return std::shared_ptr<NamedReference>(std::move(ref));


Suggested change

ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReference::Make(json.get<std::string>()));

return std::shared_ptr<NamedReference>(std::move(ref));

return NamedReference::Make(json.get<std::string>());

wgtmac · 2026-02-09T06:55:40Z

src/iceberg/expression/json_serde.cc

+  return json;
+}
+
+Result<std::shared_ptr<UnboundTransform>> UnboundTransformFromJson(


Could you please call this in the ExpressionFromJson in this PR and verify it works in the test case? It is still unclear to me whether the checks below is worth because we should already know that json is an unbound transform and those checks are performed in the ExpressionFromJson already.

evindj force-pushed the unbound_term_serde branch from 602bef1 to 684d85c Compare February 2, 2026 16:51

zhjwpku reviewed Feb 3, 2026

View reviewed changes

zhjwpku mentioned this pull request Feb 3, 2026

chore: remove unnecessary <cctype> header inclusion #554

Merged

evindj force-pushed the unbound_term_serde branch from 684d85c to 8093c92 Compare February 3, 2026 21:43

zhjwpku reviewed Feb 4, 2026

View reviewed changes

evindj force-pushed the unbound_term_serde branch 2 times, most recently from 2c7a523 to 6e6031b Compare February 5, 2026 21:56

feat: add json serde for expressions

f7a9960

evindj force-pushed the unbound_term_serde branch from 6e6031b to f7a9960 Compare February 5, 2026 22:14

zhjwpku approved these changes Feb 6, 2026

View reviewed changes

wgtmac requested changes Feb 9, 2026

View reviewed changes

	json[kType] = std::string(kTransform);
	json[kType] = kTransform;

	json[kTerm] = std::string(mutable_transform.reference()->name());
	json[kTerm] = mutable_transform.reference()->name();

	void TestJsonConversion(const T& obj, const nlohmann::json& expected_json) {
	auto json = ToJson(obj);
	EXPECT_EQ(expected_json, json) << "JSON conversion mismatch.";

	// Specialize FromJson based on type (T)
	auto obj_ex = FromJsonHelper<T>(expected_json);
	EXPECT_TRUE(obj_ex.has_value()) << "Failed to deserialize JSON.";
	EXPECT_EQ(obj, *obj_ex.value()) << "Deserialized object mismatch.";
	}

		ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReference::Make(json.get<std::string>()));
		return std::shared_ptr<NamedReference>(std::move(ref));

	ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReference::Make(json.get<std::string>()));
	return std::shared_ptr<NamedReference>(std::move(ref));
	return NamedReference::Make(json.get<std::string>());

Conversation

evindj commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants