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 implements the rest of the expression serde logic. It closely matches the java Implementation at https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java

I tried to achieve/improve on the java integration test coverage.

One gap that is not addressed in this task is that during binding, even if type information is available, a bunch of complex literals are serialized as strings but no string to specific type exist yet. I will work on that next.

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 3 times, most recently 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

evindj force-pushed the unbound_term_serde branch 5 times, most recently from a5acbaa to bb5cca5 Compare February 11, 2026 21:43

feat: add json serde for expressions

78d3930

evindj force-pushed the unbound_term_serde branch from bb5cca5 to 78d3930 Compare February 11, 2026 22:04

evindj requested a review from wgtmac February 11, 2026 22:25

evindj mentioned this pull request Feb 12, 2026

feat: bind literals with right type after serde #562

Draft

	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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

evindj commented Feb 2, 2026 •

edited

Loading