-
Notifications
You must be signed in to change notification settings - Fork 98
feat: bind literals with right type after serde #562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -298,10 +298,126 @@ Result<nlohmann::json> ToJson(const Literal& literal) { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Result<Literal> LiteralFromJson(const nlohmann::json& json, const Type* /*type*/) { | ||||||||||||||||||||||
| // TODO(gangwu): implement type-aware literal parsing equivalent to Java's | ||||||||||||||||||||||
| // SingleValueParser.fromJson(type, node). | ||||||||||||||||||||||
| return LiteralFromJson(json); | ||||||||||||||||||||||
| Result<Literal> LiteralFromJson(const nlohmann::json& json, const Type* type) { | ||||||||||||||||||||||
| // If {"type": "literal", "value": <actual>} wrapper is present, unwrap it first. | ||||||||||||||||||||||
| if (json.is_object() && json.contains(kType) && | ||||||||||||||||||||||
| json[kType].get<std::string>() == kLiteral && json.contains(kValue)) { | ||||||||||||||||||||||
| return LiteralFromJson(json[kValue], type); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| // If no type context is provided, fall back to untyped parsing. | ||||||||||||||||||||||
| if (type == nullptr) return LiteralFromJson(json); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Type-aware parsing equivalent to Java's SingleValueParser.fromJson(type, node). | ||||||||||||||||||||||
| switch (type->type_id()) { | ||||||||||||||||||||||
| case TypeId::kBoolean: | ||||||||||||||||||||||
| if (!json.is_boolean()) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a boolean value", SafeDumpJson(json)); | ||||||||||||||||||||||
| return Literal::Boolean(json.get<bool>()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kInt: | ||||||||||||||||||||||
| if (!json.is_number_integer()) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as an int value", SafeDumpJson(json)); | ||||||||||||||||||||||
| return Literal::Int(json.get<int32_t>()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kLong: | ||||||||||||||||||||||
| if (!json.is_number_integer()) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a long value", SafeDumpJson(json)); | ||||||||||||||||||||||
| return Literal::Long(json.get<int64_t>()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kFloat: | ||||||||||||||||||||||
| if (!json.is_number()) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a float value", SafeDumpJson(json)); | ||||||||||||||||||||||
| return Literal::Float(json.get<float>()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kDouble: | ||||||||||||||||||||||
| if (!json.is_number()) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a double value", SafeDumpJson(json)); | ||||||||||||||||||||||
| return Literal::Double(json.get<double>()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kString: | ||||||||||||||||||||||
| if (!json.is_string()) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a string value", SafeDumpJson(json)); | ||||||||||||||||||||||
| return Literal::String(json.get<std::string>()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // For temporal types (date, time, timestamp, timestamp_tz), we support both integer | ||||||||||||||||||||||
| // and string representations. | ||||||||||||||||||||||
| case TypeId::kDate: | ||||||||||||||||||||||
| if (json.is_number_integer()) return Literal::Date(json.get<int32_t>()); | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto days, | ||||||||||||||||||||||
| TransformUtil::ParseDay(json.get<std::string>())); | ||||||||||||||||||||||
| return Literal::Date(days); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a date value", SafeDumpJson(json)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kTime: | ||||||||||||||||||||||
| if (json.is_number_integer()) return Literal::Time(json.get<int64_t>()); | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto micros, | ||||||||||||||||||||||
| TransformUtil::ParseTime(json.get<std::string>())); | ||||||||||||||||||||||
| return Literal::Time(micros); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a time value", SafeDumpJson(json)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kTimestamp: | ||||||||||||||||||||||
| if (json.is_number_integer()) return Literal::Timestamp(json.get<int64_t>()); | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto micros, | ||||||||||||||||||||||
| TransformUtil::ParseTimestamp(json.get<std::string>())); | ||||||||||||||||||||||
| return Literal::Timestamp(micros); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a timestamp value", SafeDumpJson(json)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kTimestampTz: | ||||||||||||||||||||||
| if (json.is_number_integer()) return Literal::TimestampTz(json.get<int64_t>()); | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE( | ||||||||||||||||||||||
| auto micros, TransformUtil::ParseTimestampWithZone(json.get<std::string>())); | ||||||||||||||||||||||
| return Literal::TimestampTz(micros); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a timestamptz value", SafeDumpJson(json)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kUuid: | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto uuid, Uuid::FromString(json.get<std::string>())); | ||||||||||||||||||||||
| return Literal::UUID(uuid); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a uuid value", SafeDumpJson(json)); | ||||||||||||||||||||||
|
Comment on lines
+381
to
+385
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's just be consistent as above? Same for below. |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kBinary: | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto bytes, | ||||||||||||||||||||||
| StringUtils::HexStringToBytes(json.get<std::string>())); | ||||||||||||||||||||||
| return Literal::Binary(std::move(bytes)); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a binary value", SafeDumpJson(json)); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kFixed: { | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| const auto& fixed_type = internal::checked_cast<const FixedType&>(*type); | ||||||||||||||||||||||
| const std::string& hex = json.get<std::string>(); | ||||||||||||||||||||||
| if (hex.size() != static_cast<size_t>(fixed_type.length()) * 2) [[unlikely]] | ||||||||||||||||||||||
| return JsonParseError("Cannot parse fixed[{}]: expected {} hex chars, got {}", | ||||||||||||||||||||||
| fixed_type.length(), fixed_type.length() * 2, hex.size()); | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto bytes, StringUtils::HexStringToBytes(hex)); | ||||||||||||||||||||||
| return Literal::Fixed(std::move(bytes)); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a fixed value", SafeDumpJson(json)); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| case TypeId::kDecimal: { | ||||||||||||||||||||||
| if (json.is_string()) { | ||||||||||||||||||||||
| const auto& dec_type = internal::checked_cast<const DecimalType&>(*type); | ||||||||||||||||||||||
| ICEBERG_ASSIGN_OR_RAISE(auto dec, Decimal::FromString(json.get<std::string>())); | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to check the output scale from |
||||||||||||||||||||||
| return Literal::Decimal(dec.value(), dec_type.precision(), dec_type.scale()); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JsonParseError("Cannot parse {} as a decimal value", SafeDumpJson(json)); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| default: | ||||||||||||||||||||||
| return NotSupported("Unsupported type for literal JSON parsing: {}", | ||||||||||||||||||||||
| type->ToString()); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Result<Literal> LiteralFromJson(const nlohmann::json& json) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,11 +23,16 @@ | |
| #include <concepts> | ||
| #include <cstdint> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "iceberg/type.h" | ||
| #include "iceberg/util/checked_cast.h" | ||
| #include "iceberg/util/conversions.h" | ||
| #include "iceberg/util/decimal.h" | ||
| #include "iceberg/util/macros.h" | ||
| #include "iceberg/util/string_util.h" | ||
| #include "iceberg/util/temporal_util.h" | ||
| #include "iceberg/util/transform_util.h" | ||
|
|
||
| namespace iceberg { | ||
|
|
||
|
|
@@ -193,12 +198,36 @@ Result<Literal> LiteralCaster::CastFromString( | |
| ICEBERG_ASSIGN_OR_RAISE(auto uuid, Uuid::FromString(str_val)); | ||
| return Literal::UUID(uuid); | ||
| } | ||
| case TypeId::kDate: | ||
| case TypeId::kTime: | ||
| case TypeId::kTimestamp: | ||
| case TypeId::kTimestampTz: | ||
| return NotImplemented("Cast from String to {} is not implemented yet", | ||
| target_type->ToString()); | ||
| case TypeId::kDate: { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto days, TransformUtil::ParseDay(str_val)); | ||
| return Literal::Date(days); | ||
| } | ||
| case TypeId::kTime: { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto micros, TransformUtil::ParseTime(str_val)); | ||
| return Literal::Time(micros); | ||
| } | ||
| case TypeId::kTimestamp: { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto micros, TransformUtil::ParseTimestamp(str_val)); | ||
| return Literal::Timestamp(micros); | ||
| } | ||
| case TypeId::kTimestampTz: { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto micros, | ||
| TransformUtil::ParseTimestampWithZone(str_val)); | ||
| return Literal::TimestampTz(micros); | ||
| } | ||
| case TypeId::kBinary: { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto bytes, StringUtils::HexStringToBytes(str_val)); | ||
| return Literal::Binary(std::move(bytes)); | ||
| } | ||
| case TypeId::kFixed: { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto bytes, StringUtils::HexStringToBytes(str_val)); | ||
| return Literal::Fixed(std::move(bytes)); | ||
| } | ||
| case TypeId::kDecimal: { | ||
| const auto& dec_type = internal::checked_cast<const DecimalType&>(*target_type); | ||
| ICEBERG_ASSIGN_OR_RAISE(auto dec, Decimal::FromString(str_val)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as my other comment, we need to check the parsed scale against dec_type.scale() |
||
| return Literal::Decimal(dec.value(), dec_type.precision(), dec_type.scale()); | ||
| } | ||
| default: | ||
| return NotSupported("Cast from String to {} is not supported", | ||
| target_type->ToString()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend not support integer representation like this as the timezone processing is really tricky in C++. We cannot really trust arbitrary integers from timestamp values.