huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3462107345
##########
src/iceberg/json_serde.cc:
##########
@@ -561,9 +571,27 @@ Result<std::unique_ptr<SchemaField>> FieldFromJson(const
nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(auto name, GetJsonValue<std::string>(json, kName));
ICEBERG_ASSIGN_OR_RAISE(auto required, GetJsonValue<bool>(json, kRequired));
ICEBERG_ASSIGN_OR_RAISE(auto doc, GetJsonValueOrDefault<std::string>(json,
kDoc));
+ ICEBERG_ASSIGN_OR_RAISE(std::optional<nlohmann::json> initial_default_json,
+ GetJsonValueOptional<nlohmann::json>(json,
kInitialDefault));
+ ICEBERG_ASSIGN_OR_RAISE(std::optional<nlohmann::json> write_default_json,
+ GetJsonValueOptional<nlohmann::json>(json,
kWriteDefault));
+
+ std::shared_ptr<const Literal> initial_default;
+ if (initial_default_json.has_value()) {
+ ICEBERG_ASSIGN_OR_RAISE(Literal literal,
+ LiteralFromJson(*initial_default_json,
type.get()));
Review Comment:
Fixed in 180a9f9 — `FieldFromJson` now rejects non-UTC offsets for
`timestamptz`/`timestamptz_ns` default values (new
`TemporalUtils::IsUtcOffset`, which reuses the existing timezone-suffix parser).
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,65 @@ bool SchemaField::optional() const { return optional_; }
std::string_view SchemaField::doc() const { return doc_; }
+std::optional<std::reference_wrapper<const Literal>>
SchemaField::initial_default()
+ const {
+ if (initial_default_ == nullptr) {
+ return std::nullopt;
+ }
+ return std::cref(*initial_default_);
+}
+
+std::optional<std::reference_wrapper<const Literal>>
SchemaField::write_default() const {
+ if (write_default_ == nullptr) {
+ return std::nullopt;
+ }
+ return std::cref(*write_default_);
+}
+
+const std::shared_ptr<const Literal>& SchemaField::initial_default_ptr() const
{
+ return initial_default_;
+}
+
+const std::shared_ptr<const Literal>& SchemaField::write_default_ptr() const {
+ return write_default_;
+}
+
+namespace {
+
+Status ValidateDefault(const SchemaField& field, const Literal& value,
+ std::string_view kind) {
+ if (value.IsNull() || value.IsAboveMax() || value.IsBelowMin()) {
+ return InvalidSchema("Invalid {} value for {}: must be a non-null value",
kind,
+ field.name());
+ }
+ if (field.type() == nullptr || !field.type()->is_primitive()) {
+ return InvalidSchema(
+ "Invalid {} value for {}: default values are only supported for
primitive types",
Review Comment:
Intentional for this patch — it matches Java's current primitive-only model.
Added a comment in `ValidateDefault` marking struct/list/map single-value
defaults as a known spec gap / follow-up (180a9f9).
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,65 @@ bool SchemaField::optional() const { return optional_; }
std::string_view SchemaField::doc() const { return doc_; }
+std::optional<std::reference_wrapper<const Literal>>
SchemaField::initial_default()
+ const {
+ if (initial_default_ == nullptr) {
+ return std::nullopt;
+ }
+ return std::cref(*initial_default_);
+}
+
+std::optional<std::reference_wrapper<const Literal>>
SchemaField::write_default() const {
+ if (write_default_ == nullptr) {
+ return std::nullopt;
+ }
+ return std::cref(*write_default_);
+}
+
+const std::shared_ptr<const Literal>& SchemaField::initial_default_ptr() const
{
+ return initial_default_;
+}
+
+const std::shared_ptr<const Literal>& SchemaField::write_default_ptr() const {
+ return write_default_;
+}
+
+namespace {
+
+Status ValidateDefault(const SchemaField& field, const Literal& value,
+ std::string_view kind) {
+ if (value.IsNull() || value.IsAboveMax() || value.IsBelowMin()) {
+ return InvalidSchema("Invalid {} value for {}: must be a non-null value",
kind,
+ field.name());
+ }
+ if (field.type() == nullptr || !field.type()->is_primitive()) {
+ return InvalidSchema(
+ "Invalid {} value for {}: default values are only supported for
primitive types",
+ kind, field.name());
+ }
+ if (*value.type() != *field.type()) {
Review Comment:
Fixed in 180a9f9 — `ValidateDefault` now casts the default to the field type
via `Literal::CastTo` (e.g. int -> long) instead of requiring exact type
equality, matching Java's `Types.NestedField`. Only uncastable or out-of-range
defaults are rejected.
##########
src/iceberg/test/json_serde_test.cc:
##########
@@ -86,6 +95,66 @@ void TestJsonConversion(const T& obj, const nlohmann::json&
expected_json) {
} // namespace
+// Pins the wire format produced by ToJson(SchemaField) / FieldFromJson for
+// `initial-default` and `write-default`, including the absence of the keys
when a
+// field carries no defaults.
+TEST(JsonInternalTest, SchemaFieldDefaultValues) {
+ // Both defaults present.
+ SchemaField with_both(/*field_id=*/1, "id", int32(), /*optional=*/false,
/*doc=*/{},
+ std::make_shared<const Literal>(Literal::Int(42)),
+ std::make_shared<const Literal>(Literal::Int(7)));
+ TestJsonConversion(
+ with_both,
+
R"({"id":1,"name":"id","required":true,"type":"int","initial-default":42,"write-default":7})"_json);
+
+ // Only an initial-default; write-default must not appear in the JSON.
+ SchemaField initial_only(/*field_id=*/2, "name", string(), /*optional=*/true,
+ /*doc=*/{},
+ std::make_shared<const
Literal>(Literal::String("n/a")),
+ /*write_default=*/nullptr);
+ TestJsonConversion(
+ initial_only,
+
R"({"id":2,"name":"name","required":false,"type":"string","initial-default":"n/a"})"_json);
+
+ // No defaults; neither key may appear.
+ SchemaField no_defaults(/*field_id=*/3, "plain", int32(),
/*optional=*/false);
+ TestJsonConversion(no_defaults,
+
R"({"id":3,"name":"plain","required":true,"type":"int"})"_json);
+}
+
+// Round-trips a field carrying both defaults through ToJson -> FieldFromJson
for
+// every primitive type, exercising the per-type single-value serialization the
+// default path reuses (date/timestamp/decimal/uuid/binary have non-trivial
wire
+// encodings).
+TEST(JsonInternalTest, SchemaFieldDefaultValuesRoundTripAllTypes) {
+ ICEBERG_UNWRAP_OR_FAIL(auto uuid_value,
+
Uuid::FromString("f79c3e09-677c-4bbd-a479-3f349cb785e7"));
+ std::vector<std::pair<std::shared_ptr<Type>, Literal>> cases;
+ cases.emplace_back(boolean(), Literal::Boolean(true));
+ cases.emplace_back(int32(), Literal::Int(-7));
+ cases.emplace_back(int64(), Literal::Long(1234567890123LL));
+ cases.emplace_back(float32(), Literal::Float(1.5f));
+ cases.emplace_back(float64(), Literal::Double(2.5));
+ cases.emplace_back(date(), Literal::Date(19738));
+ cases.emplace_back(timestamp(), Literal::Timestamp(1719446400000000LL));
+ cases.emplace_back(string(), Literal::String("hello"));
+ cases.emplace_back(decimal(9, 2), Literal::Decimal(12345, 9, 2));
+ cases.emplace_back(fixed(3), Literal::Fixed({0x01, 0x02, 0x03}));
+ cases.emplace_back(binary(), Literal::Binary({0xDE, 0xAD, 0xBE, 0xEF}));
+ cases.emplace_back(uuid(), Literal::UUID(uuid_value));
Review Comment:
Done in 180a9f9 — added `time`/`timestamptz`/`timestamp_ns`/`timestamptz_ns`
to the round-trip cases, plus a negative test
(`SchemaFieldRejectsNonUtcTimestamptzDefault`) for non-UTC `timestamptz`
defaults.
--
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]
