Skip to content

Commit 8e5f13b

Browse files
wgtmacwgtmac
committed
add more error check
1 parent ef0b0fc commit 8e5f13b

File tree

3 files changed

+42
-31
lines changed

3 files changed

+42
-31
lines changed

‎src/iceberg/schema_internal.cc‎

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -488,19 +488,19 @@ namespace {
488488
}); \
489489
}
490490

491-
expected<std::shared_ptr<Type>, Error> StructTypeFromJson(const nlohmann::json& json) {
491+
expected<std::unique_ptr<Type>, Error> StructTypeFromJson(const nlohmann::json& json) {
492492
ICEBERG_CHECK_JSON_FIELD(kFields, json);
493493

494494
std::vector<SchemaField> fields;
495495
for (const auto& field_json : json[kFields]) {
496496
ICEBERG_ASSIGN_OR_RAISE(auto field, FieldFromJson(field_json));
497-
fields.push_back(std::move(field));
497+
fields.emplace_back(std::move(*field));
498498
}
499499

500-
return std::make_shared<StructType>(std::move(fields));
500+
return std::make_unique<StructType>(std::move(fields));
501501
}
502502

503-
expected<std::shared_ptr<Type>, Error> ListTypeFromJson(const nlohmann::json& json) {
503+
expected<std::unique_ptr<Type>, Error> ListTypeFromJson(const nlohmann::json& json) {
504504
ICEBERG_CHECK_JSON_FIELD(kElement, json);
505505
ICEBERG_CHECK_JSON_FIELD(kElementId, json);
506506
ICEBERG_CHECK_JSON_FIELD(kElementRequired, json);
@@ -509,12 +509,12 @@ expected<std::shared_ptr<Type>, Error> ListTypeFromJson(const nlohmann::json& js
509509
int32_t element_id = json[kElementId].get<int32_t>();
510510
bool element_required = json[kElementRequired].get<bool>();
511511

512-
return std::make_shared<ListType>(
512+
return std::make_unique<ListType>(
513513
SchemaField(element_id, std::string(ListType::kElementName),
514514
std::move(element_type), !element_required));
515515
}
516516

517-
expected<std::shared_ptr<Type>, Error> MapTypeFromJson(const nlohmann::json& json) {
517+
expected<std::unique_ptr<Type>, Error> MapTypeFromJson(const nlohmann::json& json) {
518518
ICEBERG_CHECK_JSON_FIELD(kKey, json);
519519
ICEBERG_CHECK_JSON_FIELD(kValue, json);
520520
ICEBERG_CHECK_JSON_FIELD(kKeyId, json);
@@ -531,51 +531,59 @@ expected<std::shared_ptr<Type>, Error> MapTypeFromJson(const nlohmann::json& jso
531531
/*optional=*/false);
532532
SchemaField value_field(value_id, std::string(MapType::kValueName),
533533
std::move(value_type), !value_required);
534-
return std::make_shared<MapType>(std::move(key_field), std::move(value_field));
534+
return std::make_unique<MapType>(std::move(key_field), std::move(value_field));
535535
}
536536

537537
} // namespace
538538

539-
expected<std::shared_ptr<Type>, Error> TypeFromJson(const nlohmann::json& json) {
539+
expected<std::unique_ptr<Type>, Error> TypeFromJson(const nlohmann::json& json) {
540540
if (json.is_string()) {
541541
std::string type_str = json.get<std::string>();
542542
if (type_str == "boolean") {
543-
return std::make_shared<BooleanType>();
543+
return std::make_unique<BooleanType>();
544544
} else if (type_str == "int") {
545-
return std::make_shared<IntType>();
545+
return std::make_unique<IntType>();
546546
} else if (type_str == "long") {
547-
return std::make_shared<LongType>();
547+
return std::make_unique<LongType>();
548548
} else if (type_str == "float") {
549-
return std::make_shared<FloatType>();
549+
return std::make_unique<FloatType>();
550550
} else if (type_str == "double") {
551-
return std::make_shared<DoubleType>();
551+
return std::make_unique<DoubleType>();
552552
} else if (type_str == "date") {
553-
return std::make_shared<DateType>();
553+
return std::make_unique<DateType>();
554554
} else if (type_str == "time") {
555-
return std::make_shared<TimeType>();
555+
return std::make_unique<TimeType>();
556556
} else if (type_str == "timestamp") {
557-
return std::make_shared<TimestampType>();
557+
return std::make_unique<TimestampType>();
558558
} else if (type_str == "timestamptz") {
559-
return std::make_shared<TimestampTzType>();
559+
return std::make_unique<TimestampTzType>();
560560
} else if (type_str == "string") {
561-
return std::make_shared<StringType>();
561+
return std::make_unique<StringType>();
562562
} else if (type_str == "binary") {
563-
return std::make_shared<BinaryType>();
563+
return std::make_unique<BinaryType>();
564564
} else if (type_str == "uuid") {
565-
return std::make_shared<UuidType>();
565+
return std::make_unique<UuidType>();
566566
} else if (type_str.starts_with("fixed")) {
567567
std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
568568
std::smatch match;
569569
if (std::regex_match(type_str, match, fixed_regex)) {
570-
return std::make_shared<FixedType>(std::stoi(match[1].str()));
570+
return std::make_unique<FixedType>(std::stoi(match[1].str()));
571571
}
572+
return unexpected<Error>({
573+
.kind = ErrorKind::kJsonParseError,
574+
.message = std::format("Invalid fixed type: {}", type_str),
575+
});
572576
} else if (type_str.starts_with("decimal")) {
573577
std::regex decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
574578
std::smatch match;
575579
if (std::regex_match(type_str, match, decimal_regex)) {
576-
return std::make_shared<DecimalType>(std::stoi(match[1].str()),
580+
return std::make_unique<DecimalType>(std::stoi(match[1].str()),
577581
std::stoi(match[2].str()));
578582
}
583+
return unexpected<Error>({
584+
.kind = ErrorKind::kJsonParseError,
585+
.message = std::format("Invalid decimal type: {}", type_str),
586+
});
579587
} else {
580588
return unexpected<Error>({
581589
.kind = ErrorKind::kJsonParseError,
@@ -601,7 +609,7 @@ expected<std::shared_ptr<Type>, Error> TypeFromJson(const nlohmann::json& json)
601609
}
602610
}
603611

604-
expected<SchemaField, Error> FieldFromJson(const nlohmann::json& json) {
612+
expected<std::unique_ptr<SchemaField>, Error> FieldFromJson(const nlohmann::json& json) {
605613
ICEBERG_CHECK_JSON_FIELD(kId, json);
606614
ICEBERG_CHECK_JSON_FIELD(kName, json);
607615
ICEBERG_CHECK_JSON_FIELD(kType, json);
@@ -612,18 +620,19 @@ expected<SchemaField, Error> FieldFromJson(const nlohmann::json& json) {
612620
std::string name = json[kName].get<std::string>();
613621
bool required = json[kRequired].get<bool>();
614622

615-
return SchemaField(field_id, std::move(name), std::move(type), !required);
623+
return std::make_unique<SchemaField>(field_id, std::move(name), std::move(type),
624+
!required);
616625
}
617626

618627
expected<std::unique_ptr<Schema>, Error> SchemaFromJson(const nlohmann::json& json) {
619628
ICEBERG_CHECK_JSON_FIELD(kType, json);
620629
ICEBERG_CHECK_JSON_FIELD(kSchemaId, json);
621630

622631
ICEBERG_ASSIGN_OR_RAISE(auto type, TypeFromJson(json));
623-
if (type->type_id() != TypeId::kStruct) {
632+
if (type->type_id() != TypeId::kStruct) [[unlikely]] {
624633
return unexpected<Error>({
625634
.kind = ErrorKind::kJsonParseError,
626-
.message = "Schema must be a struct type",
635+
.message = std::format("Schema must be a struct type, but got {}", json.dump()),
627636
});
628637
}
629638

‎src/iceberg/schema_internal.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,12 @@ expected<std::unique_ptr<Schema>, Error> SchemaFromJson(const nlohmann::json& js
7878
///
7979
/// \param[in] json The JSON representation of the type.
8080
/// \return The Iceberg type or an error if the conversion fails.
81-
expected<std::shared_ptr<Type>, Error> TypeFromJson(const nlohmann::json& json);
81+
expected<std::unique_ptr<Type>, Error> TypeFromJson(const nlohmann::json& json);
8282

8383
/// \brief Convert JSON to an Iceberg SchemaField.
8484
///
8585
/// \param[in] json The JSON representation of the field.
8686
/// \return The Iceberg field or an error if the conversion fails.
87-
expected<SchemaField, Error> FieldFromJson(const nlohmann::json& json);
87+
expected<std::unique_ptr<SchemaField>, Error> FieldFromJson(const nlohmann::json& json);
8888

8989
} // namespace iceberg

‎test/schema_json_test.cc‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ TEST_P(TypeJsonTest, SingleTypeRoundTrip) {
4949
auto type_result = TypeFromJson(nlohmann::json::parse(param.json));
5050
ASSERT_TRUE(type_result.has_value()) << "Failed to deserialize " << param.json
5151
<< " with error " << type_result.error().message;
52-
auto type = type_result.value();
52+
auto type = std::move(type_result.value());
5353
ASSERT_EQ(*param.type, *type);
5454
}
5555

@@ -96,14 +96,16 @@ TEST(TypeJsonTest, FromJsonWithSpaces) {
9696
auto fixed_result = TypeFromJson(nlohmann::json::parse(fixed_json));
9797
ASSERT_TRUE(fixed_result.has_value());
9898
ASSERT_EQ(fixed_result.value()->type_id(), TypeId::kFixed);
99-
auto fixed = std::dynamic_pointer_cast<FixedType>(fixed_result.value());
99+
auto fixed = dynamic_cast<FixedType*>(fixed_result.value().get());
100+
ASSERT_NE(fixed, nullptr);
100101
ASSERT_EQ(fixed->length(), 8);
101102

102103
auto decimal_json = "\"decimal( 10, 2 )\"";
103104
auto decimal_result = TypeFromJson(nlohmann::json::parse(decimal_json));
104105
ASSERT_TRUE(decimal_result.has_value());
105106
ASSERT_EQ(decimal_result.value()->type_id(), TypeId::kDecimal);
106-
auto decimal = std::dynamic_pointer_cast<DecimalType>(decimal_result.value());
107+
auto decimal = dynamic_cast<DecimalType*>(decimal_result.value().get());
108+
ASSERT_NE(decimal, nullptr);
107109
ASSERT_EQ(decimal->precision(), 10);
108110
ASSERT_EQ(decimal->scale(), 2);
109111
}

0 commit comments

Comments
 (0)