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.

src/iceberg/expression/json_serde.cc

src/iceberg/type_fwd.h

src/iceberg/test/expression_json_test.cc

src/iceberg/expression/json_serde.cc

wgtmac

Sorry for the delay and thanks for adding the whole picture! I've left some inline comments. Let me know what you think.

wgtmac · 2026-02-12T02:39:16Z

src/iceberg/expression/json_serde_internal.h

+///
+/// \param term The term to serialize (NamedReference or UnboundTransform)
+/// \return A JSON value representing the term
+ICEBERG_EXPORT nlohmann::json TermToJson(const Term& term);


Just to confirm: we cannot simply use ToJson(const Term& term) because it will confuse the function resolution?

wgtmac · 2026-02-12T07:04:25Z

src/iceberg/expression/json_serde.cc

+
+/// Template helper to create predicates from JSON with the appropriate term type
+template <typename B>
+Result<std::unique_ptr<UnboundPredicate>> MakePredicateFromJson(


Suggested change

Result<std::unique_ptr<UnboundPredicate>> MakePredicateFromJson(

Result<std::unique_ptr<UnboundPredicate>> PredicateFromJson(

To be consistent with other XyzFromJson

src/iceberg/expression/json_serde.cc

wgtmac · 2026-02-25T04:01:43Z

src/iceberg/expression/json_serde.cc

 nlohmann::json ToJson(const Expression& expr) {
  switch (expr.op()) {
    case Expression::Operation::kTrue:
      return true;


Just a heads-up that there is an inconsistency between the Rest spec and Java impl on boolean expr: apache/iceberg#14677

src/iceberg/expression/json_serde.cc

wgtmac · 2026-02-25T08:13:17Z

src/iceberg/expression/json_serde.cc

+}
+
+nlohmann::json ToJson(const UnboundTransform& transform) {
+  auto& mutable_transform = const_cast<UnboundTransform&>(transform);


Not related to this PR: I have to admit that the current api design is flawed so we have to use const_cast here. We might need to fix it in the future.

src/iceberg/expression/json_serde.cc

wgtmac · 2026-02-25T08:28:58Z

src/iceberg/expression/json_serde.cc

+  }
+}
+
+Result<Literal> LiteralFromJson(const nlohmann::json& json) {


We might need to add a const Schema* as well to follow this:

https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java#L324-L329

I decided not to add it because java always passes null for this parameter.
https://github.com/apache/iceberg/blob/d2fbe427ecec298f1600260f40fcab1b7ab2e695/core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java#L262-L268
If there is no objection, I think I will keep this as is.

Fair enough. Perhaps we can add a comment to say this discrepancy against the Java impl.

After thinking more about this, if we can pass a Schema here to bind the reference to a real data type, we can restore the literal on that type. Otherwise it may cause some troubles in the downstream to consume the filter (e.g. the actual type is decimal but double is returned for now).

There are some moving pieces related to this. For example, the casting of literals to string is not yet implemented - https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/expression/literal.cc#L196-L200. I have this another PR (main...evindj:iceberg-cpp:bound_expressions) that is supposed to addressed binding. I will add basic schema information here, but the actual casting information will be added in a different PR if that works for you.

wgtmac · 2026-02-25T08:34:09Z

src/iceberg/expression/json_serde_internal.h


 #pragma once

+#include <charconv>


Nit: <charconv> is included but never used in this header or in json_serde.cc. Please remove it.

wgtmac · 2026-02-25T08:35:23Z

src/iceberg/expression/json_serde.cc

+
+  if (IsSetOperation(op)) {
+    std::vector<Literal> literals;
+    if (!json.contains(kValues) || !json[kValues].is_array()) [[unlikely]] {


Java's predicateFromJson validates that set predicates (IN/NOT_IN) do not have a value field. This cross-validation is missing here. A malformed JSON with both value and values would be silently accepted.

wgtmac · 2026-02-25T08:36:40Z

src/iceberg/expression/json_serde.cc

+  }
+
+  // Literal predicate
+  if (!json.contains(kValue)) [[unlikely]] {


Similarly, Java validates that literal predicates (LT, EQ, etc.) do not have a values field.

src/iceberg/expression/json_serde.cc

wgtmac · 2026-02-26T09:26:07Z

Thanks for addressing the comments. Let me know when ready for review again :)

src/iceberg/expression/json_serde.cc

wgtmac

Review Report: PR #553

(Generated by gemini-cli)

📄 File: `src/iceberg/expression/json_serde.cc`

Java Counterpart: org.apache.iceberg.expressions.ExpressionParser

Parity Check: ⚠️
- Missing top-level literal expression parsing: In Java, ExpressionParser.fromJson explicitly handles expressions encoded as literal objects (e.g., {"type": "literal", "value": true}) and converts them to alwaysTrue() or alwaysFalse(). In the C++ implementation, ExpressionFromJson checks for primitive booleans and strings, but does not intercept the literal wrapper. It falls through to OperationTypeFromJson, which will return a JsonParseError since "literal" is not a known operation. You should add a check for json[kType].get<std::string>() == kLiteral inside ExpressionFromJson that validates and returns the boolean expression.
- Binding deferral: The design choice to defer binding (parsing string/long into typed equivalents) rather than doing it directly in ExpressionFromJson (as Java optionally does with a Schema argument) is a solid C++ architectural choice.
Style & Comments: ⚠️
- Good use of Modern C++ ([[unlikely]], std::ranges, std::span).
- Minor typo in an error message on line 369: "expresion JSON must be an object with a 'type' field: {}" -> should be "expression".
Logic Check: ✅
- Memory handling and move semantics are clean. LiteralFromJson gracefully handles nested literal unwrapping via recursion.
- Binary literal Base-16 encoding correctly mimics Java's BaseEncoding.base16() format.
Design & Conciseness: ✅
- The MakeTransformJson helper is a nice touch to avoid duplicating serialization logic for BoundTransform and UnboundTransform.
- Virtual methods predicate_term() and literals() in predicate.h represent a clean object-oriented approach to extracting data for serialization.
Test Quality: ⚠️
- The parameterized ExpressionJsonRoundTripTest and invalid expression tests are fantastic and comprehensive.
- Recommendation: Add a test case for {"type": "literal", "value": true} deserializing to Expression::Operation::kTrue to cover the parity issue mentioned above.

📄 Files: `src/iceberg/expression/predicate.h` & `src/iceberg/type_fwd.h`

Design & Conciseness: ✅
- Forward declarations and abstraction interfaces are correctly updated without bleeding JSON dependencies into the core expression AST.

Summary & Recommendation

Request Changes: Please address the parity gap where top-level literal wrappers {"type": "literal", "value": true} fail to parse in ExpressionFromJson, fix the minor typo, and add the corresponding test case. The overall structure and adherence to modern C++ conventions are excellent.

wgtmac · 2026-02-27T07:40:40Z

The above comment was generated automatically by my gemini-cli. I haven't finished my review yet :)

wgtmac · 2026-02-27T08:01:28Z

src/iceberg/expression/predicate.h

  bool is_unbound_predicate() const override { return true; }

+  /// \brief Returns the term of this predicate as a base Term reference.
+  virtual const Term& predicate_term() const = 0;


Suggested change

virtual const Term& predicate_term() const = 0;

virtual const Term& unbound_term() const = 0;

Not a strong opinion here but I just think predicate_term looks weird. Perhaps untyped_term, generic_term, or something else.

wgtmac · 2026-02-27T08:01:53Z

src/iceberg/expression/json_serde.cc


 namespace iceberg {
 namespace {
+// JSON field names


Suggested change

// JSON field names

wgtmac · 2026-02-27T08:01:59Z

src/iceberg/expression/json_serde.cc

+constexpr std::string_view kLeft = "left";
+constexpr std::string_view kRight = "right";
+constexpr std::string_view kChild = "child";
 // Expression type strings


Suggested change

// Expression type strings

wgtmac · 2026-02-27T08:07:20Z

src/iceberg/expression/json_serde.cc

+    std::vector<Literal> literals;
+    if (!json.contains(kValues) || !json[kValues].is_array() || json.contains(kValue))
+        [[unlikely]] {
+      return JsonParseError("Missing or invalid 'values' field for set operation: {}",


The error msg is out of date

wgtmac · 2026-02-27T08:07:41Z

src/iceberg/expression/json_serde.cc

+
+  // Literal predicate
+  if (!json.contains(kValue) || json.contains(kValues)) [[unlikely]] {
+    return JsonParseError("Missing 'value' field for literal predicate: {}",


wgtmac · 2026-02-27T08:21:40Z

src/iceberg/expression/json_serde.cc

+}
+
+Result<nlohmann::json> ToJson(const BoundReference& ref) {
+  return nlohmann::json(ref.name());


wgtmac · 2026-02-27T08:32:55Z

src/iceberg/expression/json_serde.cc

+  }
+}
+
+Result<Literal> LiteralFromJson(const nlohmann::json& json) {


After thinking more about this, if we can pass a Schema here to bind the reference to a real data type, we can restore the literal on that type. Otherwise it may cause some troubles in the downstream to consume the filter (e.g. the actual type is decimal but double is returned for now).

wgtmac · 2026-02-27T08:35:39Z

src/iceberg/expression/json_serde.cc

+        return ToJson(internal::checked_cast<const UnboundTransform&>(term));
+      return ToJson(internal::checked_cast<const BoundTransform&>(term));
+    default:
+      return NotSupported("Unsupported term kind for JSON serialization");


Let's include more detail in the error msg?

wgtmac · 2026-02-27T08:41:44Z

src/iceberg/expression/json_serde.cc

+
+  const auto& term_json = json[kTerm];
+
+  if (IsTransformTerm(term_json)) {


Should we add a UnboundTermFromJson and then wrap UnboundTransformFromJson and NamedReferenceFromJson in it? UnboundTermFromJson can call itself recursively. I'm not sure if it is possible to produce transform like identity(identity(col)).

wgtmac · 2026-02-27T08:49:30Z

src/iceberg/expression/json_serde.cc

+      return Not::Make(std::move(child));
+    }
+    default:
+      // All other operations are predicates however since binding does not happen during


All other operations are predicates

Is it true? Should we try to check if this is an aggregate?

wgtmac · 2026-02-27T08:57:11Z

src/iceberg/test/expression_json_test.cc

+  ICEBERG_UNWRAP_OR_FAIL(auto result2, ToJson(*expr));
+  EXPECT_EQ(result2, expected);


Suggested change

ICEBERG_UNWRAP_OR_FAIL(auto result2, ToJson(*expr));

EXPECT_EQ(result2, expected);

ICEBERG_UNWRAP_OR_FAIL(auto result, ToJson(*expr));

EXPECT_EQ(result, expected);

wgtmac · 2026-02-27T08:58:54Z

src/iceberg/test/expression_json_test.cc

+  EXPECT_EQ(round_trip, param.json);
+}
+
+INSTANTIATE_TEST_SUITE_P(


Thanks for adding these test cases!

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

src/iceberg/expression/json_serde.cc Outdated Show resolved Hide resolved

src/iceberg/expression/json_serde.cc Outdated Show resolved Hide resolved

src/iceberg/expression/json_serde.cc Outdated Show resolved Hide resolved

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

src/iceberg/expression/json_serde.cc Outdated Show resolved Hide resolved

src/iceberg/expression/json_serde.cc Outdated Show resolved Hide resolved

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

wgtmac requested changes Feb 25, 2026

View reviewed changes

wgtmac reviewed Feb 25, 2026

View reviewed changes

evindj force-pushed the unbound_term_serde branch 2 times, most recently from ae55d26 to 1ca8c76 Compare February 25, 2026 18:05

evindj force-pushed the unbound_term_serde branch 3 times, most recently from a7aebcf to f1c6d91 Compare February 27, 2026 04:03

evindj requested a review from wgtmac February 27, 2026 04:43

evindj commented Feb 27, 2026

View reviewed changes

src/iceberg/expression/json_serde.cc Outdated Show resolved Hide resolved

wgtmac requested changes Feb 27, 2026

View reviewed changes

evindj force-pushed the unbound_term_serde branch from f1c6d91 to ecef4c4 Compare February 27, 2026 08:44

wgtmac reviewed Feb 27, 2026

View reviewed changes

feat: add json serde for expressions

cadf471

evindj force-pushed the unbound_term_serde branch from ecef4c4 to cadf471 Compare February 27, 2026 23:54

	Result<std::unique_ptr<UnboundPredicate>> MakePredicateFromJson(
	Result<std::unique_ptr<UnboundPredicate>> PredicateFromJson(

	virtual const Term& predicate_term() const = 0;
	virtual const Term& unbound_term() const = 0;


		const auto& term_json = json[kTerm];

		if (IsTransformTerm(term_json)) {

		ICEBERG_UNWRAP_OR_FAIL(auto result2, ToJson(*expr));
		EXPECT_EQ(result2, expected);

Conversation

evindj commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evindj Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

evindj Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

wgtmac commented Feb 26, 2026

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Review Report: PR #553

📄 File: src/iceberg/expression/json_serde.cc

📄 Files: src/iceberg/expression/predicate.h & src/iceberg/type_fwd.h

Summary & Recommendation

Uh oh!

wgtmac commented Feb 27, 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

evindj commented Feb 2, 2026 •

edited

Loading

evindj Feb 25, 2026 •

edited

Loading

evindj Feb 27, 2026 •

edited

Loading

📄 File: `src/iceberg/expression/json_serde.cc`

📄 Files: `src/iceberg/expression/predicate.h` & `src/iceberg/type_fwd.h`