wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3457244245
##########
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:
Default-value parsing should enforce the JSON single-value rule for
`timestamptz` and `timestamptz_ns`. The shared timestamp parser accepts offsets
such as `+05:00`, but the spec and Java `SingleValueParser` only accept UTC
(`+00:00`) for these default values. As written, C++ can accept schema metadata
that Java rejects and then silently normalize it on write.
##########
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:
This rejects all non-primitive defaults. The spec allows JSON single-value
defaults for structs/lists/maps, and a struct field may have a non-null empty
struct default (`{}`) while sub-field defaults live in field metadata. If C++
is intentionally matching the current Java model for this first patch, please
call out that this remains a spec gap; otherwise this will reject legal v3
schema metadata.
##########
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:
Java casts the provided default literal to the field type in
`Types.NestedField` (`Literal.of(34)` can become a long/date default, string
literals can become date/time/timestamp/uuid, etc.). This check requires exact
type equality, so schemas built through the C++ API can be rejected even when
Java accepts and normalizes the same default.
##########
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:
This round-trip list should include `time`, `timestamptz`, `timestamp_ns`,
and `timestamptz_ns`, plus a negative case for non-UTC timestamptz defaults.
Those are the encodings most likely to drift from Java/spec behavior; the
current cases do not cover the offset rule above.
--
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]
