Skip to content

Commit a531643

Browse files
wgtmacwgtmac
committed
add some fixes
1 parent cadf471 commit a531643

File tree

2 files changed

+47
-55
lines changed

2 files changed

+47
-55
lines changed

‎src/iceberg/expression/json_serde.cc‎

Lines changed: 46 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,11 @@
1717
* under the License.
1818
*/
1919

20-
#include <ranges>
2120
#include <string>
2221
#include <vector>
2322

2423
#include <nlohmann/json.hpp>
2524

26-
#include "iceberg/expression/binder.h"
2725
#include "iceberg/expression/json_serde_internal.h"
2826
#include "iceberg/expression/literal.h"
2927
#include "iceberg/expression/predicate.h"
@@ -33,6 +31,7 @@
3331
#include "iceberg/util/checked_cast.h"
3432
#include "iceberg/util/json_util_internal.h"
3533
#include "iceberg/util/macros.h"
34+
#include "iceberg/util/string_util.h"
3635
#include "iceberg/util/transform_util.h"
3736

3837
namespace iceberg {
@@ -184,18 +183,30 @@ Result<Expression::Operation> OperationTypeFromJson(const nlohmann::json& json)
184183
if (typeStr == kMin) return Expression::Operation::kMin;
185184
if (typeStr == kMax) return Expression::Operation::kMax;
186185

187-
return JsonParseError("Unknown expression type: {}", typeStr);
186+
return JsonParseError("Unknown expression operation: '{}'", typeStr);
188187
}
189188

190189
nlohmann::json ToJson(Expression::Operation op) {
191190
std::string json(ToString(op));
192191
std::ranges::transform(json, json.begin(), [](unsigned char c) -> char {
193192
return (c == '_') ? '-' : static_cast<char>(std::tolower(c));
194193
});
195-
return nlohmann::json(std::move(json));
194+
return json;
196195
}
197196

198-
nlohmann::json ToJson(const NamedReference& ref) { return nlohmann::json(ref.name()); }
197+
nlohmann::json ToJson(const NamedReference& ref) { return ref.name(); }
198+
199+
nlohmann::json ToJson(const UnboundTransform& transform) {
200+
auto& mut = const_cast<UnboundTransform&>(transform);
201+
return MakeTransformJson(transform.transform()->ToString(), mut.reference()->name());
202+
}
203+
204+
nlohmann::json ToJson(const BoundReference& ref) { return ref.name(); }
205+
206+
nlohmann::json ToJson(const BoundTransform& transform) {
207+
auto& mut = const_cast<BoundTransform&>(transform);
208+
return MakeTransformJson(transform.transform()->ToString(), mut.reference()->name());
209+
}
199210

200211
Result<std::unique_ptr<NamedReference>> NamedReferenceFromJson(
201212
const nlohmann::json& json) {
@@ -209,28 +220,16 @@ Result<std::unique_ptr<NamedReference>> NamedReferenceFromJson(
209220
return NamedReference::Make(json.get<std::string>());
210221
}
211222

212-
nlohmann::json ToJson(const UnboundTransform& transform) {
213-
auto& mut = const_cast<UnboundTransform&>(transform);
214-
return MakeTransformJson(transform.transform()->ToString(), mut.reference()->name());
215-
}
216-
217-
nlohmann::json ToJson(const BoundReference& ref) { return nlohmann::json(ref.name()); }
218-
219-
nlohmann::json ToJson(const BoundTransform& transform) {
220-
auto& mut = const_cast<BoundTransform&>(transform);
221-
return MakeTransformJson(transform.transform()->ToString(), mut.reference()->name());
222-
}
223-
224223
Result<std::unique_ptr<UnboundTransform>> UnboundTransformFromJson(
225224
const nlohmann::json& json) {
226-
if (IsTransformTerm(json)) {
227-
ICEBERG_ASSIGN_OR_RAISE(auto transform_str,
228-
GetJsonValue<std::string>(json, kTransform));
229-
ICEBERG_ASSIGN_OR_RAISE(auto transform, TransformFromString(transform_str));
230-
ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReferenceFromJson(json[kTerm]));
231-
return UnboundTransform::Make(std::move(ref), std::move(transform));
225+
if (!IsTransformTerm(json)) {
226+
return JsonParseError("Invalid unbound transform: {}", SafeDumpJson(json));
232227
}
233-
return JsonParseError("Invalid unbound transform json: {}", SafeDumpJson(json));
228+
ICEBERG_ASSIGN_OR_RAISE(auto transform_str,
229+
GetJsonValue<std::string>(json, kTransform));
230+
ICEBERG_ASSIGN_OR_RAISE(auto transform, TransformFromString(transform_str));
231+
ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReferenceFromJson(json[kTerm]));
232+
return UnboundTransform::Make(std::move(ref), std::move(transform));
234233
}
235234

236235
Result<nlohmann::json> ToJson(const Literal& literal) {
@@ -274,9 +273,8 @@ Result<nlohmann::json> ToJson(const Literal& literal) {
274273
}
275274
return nlohmann::json(std::move(hex));
276275
}
277-
case TypeId::kDecimal: {
276+
case TypeId::kDecimal:
278277
return nlohmann::json(literal.ToString());
279-
}
280278
case TypeId::kUuid:
281279
return nlohmann::json(std::get<Uuid>(value).ToString());
282280
default:
@@ -304,38 +302,34 @@ Result<Literal> LiteralFromJson(const nlohmann::json& json) {
304302
return Literal::Double(json.get<double>());
305303
}
306304
if (json.is_string()) {
307-
// All strings are returned as String literals.
308-
// Conversion to binary/date/time/etc. happens during binding
309-
// when schema type information is available.
310305
return Literal::String(json.get<std::string>());
311306
}
312-
return JsonParseError("Unsupported literal JSON type");
307+
return JsonParseError("Unsupported literal JSON: {}", SafeDumpJson(json));
313308
}
314309

315310
Result<nlohmann::json> ToJson(const Term& term) {
316311
switch (term.kind()) {
317312
case Term::Kind::kReference:
318-
if (term.is_unbound())
313+
if (term.is_unbound()) {
319314
return ToJson(internal::checked_cast<const NamedReference&>(term));
315+
}
320316
return ToJson(internal::checked_cast<const BoundReference&>(term));
321317
case Term::Kind::kTransform:
322-
if (term.is_unbound())
318+
if (term.is_unbound()) {
323319
return ToJson(internal::checked_cast<const UnboundTransform&>(term));
320+
}
324321
return ToJson(internal::checked_cast<const BoundTransform&>(term));
325322
default:
326-
return NotSupported(
327-
"Unsupported term kind for JSON serialization only reference and transform "
328-
"terms are supported");
323+
return NotSupported("Unsupported term for JSON serialization: {}", term.ToString());
329324
}
330325
}
331326

332327
Result<nlohmann::json> ToJson(const UnboundPredicate& pred) {
333328
nlohmann::json json;
334329
json[kType] = ToJson(pred.op());
335-
336330
ICEBERG_ASSIGN_OR_RAISE(json[kTerm], ToJson(pred.unbound_term()));
337-
std::span<const Literal> literals = pred.literals();
338331

332+
std::span<const Literal> literals = pred.literals();
339333
if (IsSetOperation(pred.op())) {
340334
nlohmann::json values = nlohmann::json::array();
341335
for (const auto& lit : literals) {
@@ -344,8 +338,9 @@ Result<nlohmann::json> ToJson(const UnboundPredicate& pred) {
344338
}
345339
json[kValues] = std::move(values);
346340
} else if (!literals.empty()) {
347-
ICEBERG_DCHECK(literals.size() == 1,
348-
"Expected exactly one literal for non-set predicate");
341+
ICEBERG_CHECK(literals.size() == 1,
342+
"Expected exactly one literal for non-set predicate but got {}: {}",
343+
literals.size(), pred.ToString());
349344
ICEBERG_ASSIGN_OR_RAISE(json[kValue], ToJson(literals[0]));
350345
}
351346
return json;
@@ -378,7 +373,6 @@ Result<std::unique_ptr<UnboundPredicate>> UnboundPredicateFromJson(
378373
SafeDumpJson(json));
379374
}
380375
ICEBERG_ASSIGN_OR_RAISE(auto op, OperationTypeFromJson(json[kType]));
381-
382376
const auto& term_json = json[kTerm];
383377

384378
if (IsTransformTerm(term_json)) {
@@ -399,18 +393,17 @@ Result<std::shared_ptr<Expression>> ExpressionFromJson(const nlohmann::json& jso
399393
: internal::checked_pointer_cast<Expression>(False::Instance());
400394
}
401395
if (json.is_string()) {
402-
auto s = json.get<std::string>();
403-
std::ranges::transform(s, s.begin(), [](unsigned char c) -> char {
404-
return static_cast<char>(std::tolower(c));
405-
});
406-
if (s == kTrue) return internal::checked_pointer_cast<Expression>(True::Instance());
407-
if (s == kFalse) return internal::checked_pointer_cast<Expression>(False::Instance());
396+
auto s = StringUtils::ToLower(json.get<std::string>());
397+
if (s == kTrue) return True::Instance();
398+
if (s == kFalse) return False::Instance();
399+
return JsonParseError("Unknown expression string constant: {}", s);
408400
}
409401

410402
if (!json.is_object() || !json.contains(kType)) [[unlikely]] {
411-
return JsonParseError("expression JSON must be an object with a 'type' field: {}",
403+
return JsonParseError("Expression JSON must be an object with a 'type' field: {}",
412404
SafeDumpJson(json));
413405
}
406+
414407
if (json[kType].get<std::string>() == kLiteral) {
415408
if (!json.contains(kValue) || !json[kValue].is_boolean()) [[unlikely]] {
416409
return JsonParseError(
@@ -497,10 +490,12 @@ Result<nlohmann::json> ToJson(const Expression& expr) {
497490
return json;
498491
}
499492
default:
500-
if (expr.is_unbound_predicate())
501-
return ToJson(internal::checked_cast<const UnboundPredicate&>(expr));
502-
if (expr.is_bound_predicate())
503-
return ToJson(internal::checked_cast<const BoundPredicate&>(expr));
493+
if (expr.is_unbound_predicate()) {
494+
return ToJson(dynamic_cast<const UnboundPredicate&>(expr));
495+
}
496+
if (expr.is_bound_predicate()) {
497+
return ToJson(dynamic_cast<const BoundPredicate&>(expr));
498+
}
504499
return NotSupported("Unsupported expression type for JSON serialization");
505500
}
506501
}

‎src/iceberg/test/expression_json_test.cc‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,9 @@
3030
#include "iceberg/expression/json_serde_internal.h"
3131
#include "iceberg/expression/literal.h"
3232
#include "iceberg/expression/predicate.h"
33-
#include "iceberg/expression/term.h"
3433
#include "iceberg/schema.h"
3534
#include "iceberg/test/matchers.h"
36-
#include "iceberg/transform.h"
3735
#include "iceberg/type.h"
38-
#include "iceberg/util/uuid.h"
3936

4037
namespace iceberg {
4138

@@ -216,7 +213,7 @@ INSTANTIATE_TEST_SUITE_P(
216213
InvalidExpressionParam{"NotBooleanOrObject", 42, "an object with a 'type'"},
217214
InvalidExpressionParam{"UnknownOperationType",
218215
{{"type", "illegal"}, {"term", "col"}},
219-
"Unknown expression type"},
216+
"Unknown expression operation"},
220217
InvalidExpressionParam{
221218
"AndMissingLeft",
222219
{{"type", "and"}, {"right", {{"type", "is-null"}, {"term", "col"}}}},

0 commit comments

Comments
 (0)