wgtmac commented on code in PR #752:
URL: https://github.com/apache/iceberg-cpp/pull/752#discussion_r3453072970
##########
src/iceberg/type.cc:
##########
@@ -354,6 +359,68 @@ TypeId UnknownType::type_id() const { return kTypeId; }
std::string UnknownType::ToString() const { return "unknown"; }
bool UnknownType::Equals(const Type& other) const { return other.type_id() ==
kTypeId; }
+GeometryType::GeometryType(std::string crs) {
+ ICEBERG_CHECK_OR_DIE(!crs.empty(), "GeometryType: CRS cannot be empty");
Review Comment:
Why not simply use default crs when input is empty?
##########
src/iceberg/json_serde.cc:
##########
@@ -486,58 +492,97 @@ Result<std::unique_ptr<Type>> MapTypeFromJson(const
nlohmann::json& json) {
Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
if (json.is_string()) {
std::string type_str = json.get<std::string>();
- if (type_str == "boolean") {
+ std::string lower_type_str = StringUtils::ToLower(type_str);
+ if (lower_type_str == "boolean") {
return std::make_unique<BooleanType>();
- } else if (type_str == "int") {
+ } else if (lower_type_str == "int") {
return std::make_unique<IntType>();
- } else if (type_str == "long") {
+ } else if (lower_type_str == "long") {
return std::make_unique<LongType>();
- } else if (type_str == "float") {
+ } else if (lower_type_str == "float") {
return std::make_unique<FloatType>();
- } else if (type_str == "double") {
+ } else if (lower_type_str == "double") {
return std::make_unique<DoubleType>();
- } else if (type_str == "date") {
+ } else if (lower_type_str == "date") {
return std::make_unique<DateType>();
- } else if (type_str == "time") {
+ } else if (lower_type_str == "time") {
return std::make_unique<TimeType>();
- } else if (type_str == "timestamp") {
+ } else if (lower_type_str == "timestamp") {
return std::make_unique<TimestampType>();
- } else if (type_str == "timestamptz") {
+ } else if (lower_type_str == "timestamptz") {
return std::make_unique<TimestampTzType>();
- } else if (type_str == "timestamp_ns") {
+ } else if (lower_type_str == "timestamp_ns") {
return std::make_unique<TimestampNsType>();
- } else if (type_str == "timestamptz_ns") {
+ } else if (lower_type_str == "timestamptz_ns") {
return std::make_unique<TimestampTzNsType>();
- } else if (type_str == "string") {
+ } else if (lower_type_str == "string") {
return std::make_unique<StringType>();
- } else if (type_str == "binary") {
+ } else if (lower_type_str == "binary") {
return std::make_unique<BinaryType>();
- } else if (type_str == "uuid") {
+ } else if (lower_type_str == "uuid") {
return std::make_unique<UuidType>();
- } else if (type_str == "unknown") {
+ } else if (lower_type_str == "unknown") {
return std::make_unique<UnknownType>();
- } else if (type_str.starts_with("fixed")) {
- std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
+ } else if (lower_type_str == "variant") {
+ return std::make_unique<VariantType>();
+ } else if (lower_type_str.starts_with("fixed")) {
+ static const std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
std::smatch match;
- if (std::regex_match(type_str, match, fixed_regex)) {
+ if (std::regex_match(lower_type_str, match, fixed_regex)) {
ICEBERG_ASSIGN_OR_RAISE(auto length,
StringUtils::ParseNumber<int32_t>(match[1].str()));
return std::make_unique<FixedType>(length);
}
return JsonParseError("Invalid fixed type: {}", type_str);
- } else if (type_str.starts_with("decimal")) {
- std::regex decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
+ } else if (lower_type_str.starts_with("decimal")) {
+ static const std::regex
decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
Review Comment:
```suggestion
static const std::regex
kDecimalRegex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
```
##########
src/iceberg/transform_function.cc:
##########
@@ -40,7 +40,9 @@ std::shared_ptr<Type> IdentityTransform::ResultType() const {
return source_type
Result<std::unique_ptr<TransformFunction>> IdentityTransform::Make(
std::shared_ptr<Type> const& source_type) {
- if (!source_type || !source_type->is_primitive()) {
+ if (!source_type || !source_type->is_primitive() ||
+ source_type->type_id() == TypeId::kGeometry ||
+ source_type->type_id() == TypeId::kGeography) {
Review Comment:
Should we keep variant type as unsupported until the spec change has been
approved?
##########
src/iceberg/json_serde.cc:
##########
@@ -486,58 +492,97 @@ Result<std::unique_ptr<Type>> MapTypeFromJson(const
nlohmann::json& json) {
Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
if (json.is_string()) {
std::string type_str = json.get<std::string>();
- if (type_str == "boolean") {
+ std::string lower_type_str = StringUtils::ToLower(type_str);
+ if (lower_type_str == "boolean") {
return std::make_unique<BooleanType>();
- } else if (type_str == "int") {
+ } else if (lower_type_str == "int") {
return std::make_unique<IntType>();
- } else if (type_str == "long") {
+ } else if (lower_type_str == "long") {
return std::make_unique<LongType>();
- } else if (type_str == "float") {
+ } else if (lower_type_str == "float") {
return std::make_unique<FloatType>();
- } else if (type_str == "double") {
+ } else if (lower_type_str == "double") {
return std::make_unique<DoubleType>();
- } else if (type_str == "date") {
+ } else if (lower_type_str == "date") {
return std::make_unique<DateType>();
- } else if (type_str == "time") {
+ } else if (lower_type_str == "time") {
return std::make_unique<TimeType>();
- } else if (type_str == "timestamp") {
+ } else if (lower_type_str == "timestamp") {
return std::make_unique<TimestampType>();
- } else if (type_str == "timestamptz") {
+ } else if (lower_type_str == "timestamptz") {
return std::make_unique<TimestampTzType>();
- } else if (type_str == "timestamp_ns") {
+ } else if (lower_type_str == "timestamp_ns") {
return std::make_unique<TimestampNsType>();
- } else if (type_str == "timestamptz_ns") {
+ } else if (lower_type_str == "timestamptz_ns") {
return std::make_unique<TimestampTzNsType>();
- } else if (type_str == "string") {
+ } else if (lower_type_str == "string") {
return std::make_unique<StringType>();
- } else if (type_str == "binary") {
+ } else if (lower_type_str == "binary") {
return std::make_unique<BinaryType>();
- } else if (type_str == "uuid") {
+ } else if (lower_type_str == "uuid") {
return std::make_unique<UuidType>();
- } else if (type_str == "unknown") {
+ } else if (lower_type_str == "unknown") {
return std::make_unique<UnknownType>();
- } else if (type_str.starts_with("fixed")) {
- std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
+ } else if (lower_type_str == "variant") {
+ return std::make_unique<VariantType>();
+ } else if (lower_type_str.starts_with("fixed")) {
+ static const std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
std::smatch match;
- if (std::regex_match(type_str, match, fixed_regex)) {
+ if (std::regex_match(lower_type_str, match, fixed_regex)) {
ICEBERG_ASSIGN_OR_RAISE(auto length,
StringUtils::ParseNumber<int32_t>(match[1].str()));
return std::make_unique<FixedType>(length);
}
return JsonParseError("Invalid fixed type: {}", type_str);
- } else if (type_str.starts_with("decimal")) {
- std::regex decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
+ } else if (lower_type_str.starts_with("decimal")) {
+ static const std::regex
decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
std::smatch match;
- if (std::regex_match(type_str, match, decimal_regex)) {
+ if (std::regex_match(lower_type_str, match, decimal_regex)) {
ICEBERG_ASSIGN_OR_RAISE(auto precision,
StringUtils::ParseNumber<int32_t>(match[1].str()));
ICEBERG_ASSIGN_OR_RAISE(auto scale,
StringUtils::ParseNumber<int32_t>(match[2].str()));
return std::make_unique<DecimalType>(precision, scale);
}
return JsonParseError("Invalid decimal type: {}", type_str);
+ } else if (lower_type_str.starts_with("geometry")) {
+ static const std::regex
geometry_regex(R"(geometry\s*(?:\(\s*([^)]*?)\s*\))?)",
Review Comment:
ditto
##########
src/iceberg/json_serde.cc:
##########
@@ -486,58 +492,97 @@ Result<std::unique_ptr<Type>> MapTypeFromJson(const
nlohmann::json& json) {
Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
if (json.is_string()) {
std::string type_str = json.get<std::string>();
- if (type_str == "boolean") {
+ std::string lower_type_str = StringUtils::ToLower(type_str);
+ if (lower_type_str == "boolean") {
return std::make_unique<BooleanType>();
- } else if (type_str == "int") {
+ } else if (lower_type_str == "int") {
return std::make_unique<IntType>();
- } else if (type_str == "long") {
+ } else if (lower_type_str == "long") {
return std::make_unique<LongType>();
- } else if (type_str == "float") {
+ } else if (lower_type_str == "float") {
return std::make_unique<FloatType>();
- } else if (type_str == "double") {
+ } else if (lower_type_str == "double") {
return std::make_unique<DoubleType>();
- } else if (type_str == "date") {
+ } else if (lower_type_str == "date") {
return std::make_unique<DateType>();
- } else if (type_str == "time") {
+ } else if (lower_type_str == "time") {
return std::make_unique<TimeType>();
- } else if (type_str == "timestamp") {
+ } else if (lower_type_str == "timestamp") {
return std::make_unique<TimestampType>();
- } else if (type_str == "timestamptz") {
+ } else if (lower_type_str == "timestamptz") {
return std::make_unique<TimestampTzType>();
- } else if (type_str == "timestamp_ns") {
+ } else if (lower_type_str == "timestamp_ns") {
return std::make_unique<TimestampNsType>();
- } else if (type_str == "timestamptz_ns") {
+ } else if (lower_type_str == "timestamptz_ns") {
return std::make_unique<TimestampTzNsType>();
- } else if (type_str == "string") {
+ } else if (lower_type_str == "string") {
return std::make_unique<StringType>();
- } else if (type_str == "binary") {
+ } else if (lower_type_str == "binary") {
return std::make_unique<BinaryType>();
- } else if (type_str == "uuid") {
+ } else if (lower_type_str == "uuid") {
return std::make_unique<UuidType>();
- } else if (type_str == "unknown") {
+ } else if (lower_type_str == "unknown") {
return std::make_unique<UnknownType>();
- } else if (type_str.starts_with("fixed")) {
- std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
+ } else if (lower_type_str == "variant") {
+ return std::make_unique<VariantType>();
+ } else if (lower_type_str.starts_with("fixed")) {
+ static const std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
std::smatch match;
- if (std::regex_match(type_str, match, fixed_regex)) {
+ if (std::regex_match(lower_type_str, match, fixed_regex)) {
ICEBERG_ASSIGN_OR_RAISE(auto length,
StringUtils::ParseNumber<int32_t>(match[1].str()));
return std::make_unique<FixedType>(length);
}
return JsonParseError("Invalid fixed type: {}", type_str);
- } else if (type_str.starts_with("decimal")) {
- std::regex decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
+ } else if (lower_type_str.starts_with("decimal")) {
+ static const std::regex
decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))");
std::smatch match;
- if (std::regex_match(type_str, match, decimal_regex)) {
+ if (std::regex_match(lower_type_str, match, decimal_regex)) {
ICEBERG_ASSIGN_OR_RAISE(auto precision,
StringUtils::ParseNumber<int32_t>(match[1].str()));
ICEBERG_ASSIGN_OR_RAISE(auto scale,
StringUtils::ParseNumber<int32_t>(match[2].str()));
return std::make_unique<DecimalType>(precision, scale);
}
return JsonParseError("Invalid decimal type: {}", type_str);
+ } else if (lower_type_str.starts_with("geometry")) {
+ static const std::regex
geometry_regex(R"(geometry\s*(?:\(\s*([^)]*?)\s*\))?)",
+ std::regex_constants::icase);
+ std::smatch match;
+ if (std::regex_match(type_str, match, geometry_regex)) {
+ if (match[1].matched) {
+ auto crs = match[1].str();
+ if (crs.empty()) {
+ return JsonParseError("Invalid geometry type: {}", type_str);
+ }
+ return std::make_unique<GeometryType>(std::move(crs));
+ }
+ return std::make_unique<GeometryType>();
+ }
+ return JsonParseError("Invalid geometry type: {}", type_str);
+ } else if (lower_type_str.starts_with("geography")) {
+ static const std::regex geography_regex(
Review Comment:
ditto
##########
src/iceberg/json_serde.cc:
##########
@@ -486,58 +492,97 @@ Result<std::unique_ptr<Type>> MapTypeFromJson(const
nlohmann::json& json) {
Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
if (json.is_string()) {
std::string type_str = json.get<std::string>();
- if (type_str == "boolean") {
+ std::string lower_type_str = StringUtils::ToLower(type_str);
+ if (lower_type_str == "boolean") {
return std::make_unique<BooleanType>();
- } else if (type_str == "int") {
+ } else if (lower_type_str == "int") {
return std::make_unique<IntType>();
- } else if (type_str == "long") {
+ } else if (lower_type_str == "long") {
return std::make_unique<LongType>();
- } else if (type_str == "float") {
+ } else if (lower_type_str == "float") {
return std::make_unique<FloatType>();
- } else if (type_str == "double") {
+ } else if (lower_type_str == "double") {
return std::make_unique<DoubleType>();
- } else if (type_str == "date") {
+ } else if (lower_type_str == "date") {
return std::make_unique<DateType>();
- } else if (type_str == "time") {
+ } else if (lower_type_str == "time") {
return std::make_unique<TimeType>();
- } else if (type_str == "timestamp") {
+ } else if (lower_type_str == "timestamp") {
return std::make_unique<TimestampType>();
- } else if (type_str == "timestamptz") {
+ } else if (lower_type_str == "timestamptz") {
return std::make_unique<TimestampTzType>();
- } else if (type_str == "timestamp_ns") {
+ } else if (lower_type_str == "timestamp_ns") {
return std::make_unique<TimestampNsType>();
- } else if (type_str == "timestamptz_ns") {
+ } else if (lower_type_str == "timestamptz_ns") {
return std::make_unique<TimestampTzNsType>();
- } else if (type_str == "string") {
+ } else if (lower_type_str == "string") {
return std::make_unique<StringType>();
- } else if (type_str == "binary") {
+ } else if (lower_type_str == "binary") {
return std::make_unique<BinaryType>();
- } else if (type_str == "uuid") {
+ } else if (lower_type_str == "uuid") {
return std::make_unique<UuidType>();
- } else if (type_str == "unknown") {
+ } else if (lower_type_str == "unknown") {
return std::make_unique<UnknownType>();
- } else if (type_str.starts_with("fixed")) {
- std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
+ } else if (lower_type_str == "variant") {
+ return std::make_unique<VariantType>();
+ } else if (lower_type_str.starts_with("fixed")) {
+ static const std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])");
Review Comment:
```suggestion
static const std::regex kFixedRegex(R"(fixed\[\s*(\d+)\s*\])");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
