huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3418362976
##########
src/iceberg/schema_field.h:
##########
@@ -100,6 +142,11 @@ class ICEBERG_EXPORT SchemaField : public
iceberg::util::Formattable {
std::shared_ptr<Type> type_;
bool optional_;
std::string doc_;
+ // Default values are owned by this field and never mutated after being set;
copies
+ // of the field share the same payload (reference-counted) instead of
deep-copying,
+ // like `type_` above. Sharing is unobservable because the payload is
immutable.
+ std::shared_ptr<const Literal> initial_default_;
+ std::shared_ptr<const Literal> write_default_;
Review Comment:
Good catch, confirmed. Defaults are now constructor args, and
`ReassignField` passes the source field's
`initial_default_ptr()`/`write_default_ptr()` through, so they're shared with
the reassigned field, not lost. Added `ReassignIdsPreservesDefaultValues`.
##########
src/iceberg/json_serde.cc:
##########
@@ -552,9 +562,23 @@ 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));
-
- return std::make_unique<SchemaField>(field_id, std::move(name),
std::move(type),
- !required, doc);
+ 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));
+
+ SchemaField field(field_id, std::move(name), std::move(type), !required,
doc);
+ if (initial_default_json.has_value()) {
+ ICEBERG_ASSIGN_OR_RAISE(Literal literal,
+ LiteralFromJson(*initial_default_json,
field.type().get()));
+ field = field.WithInitialDefault(std::move(literal));
+ }
+ if (write_default_json.has_value()) {
+ ICEBERG_ASSIGN_OR_RAISE(Literal literal,
+ LiteralFromJson(*write_default_json,
field.type().get()));
+ field = field.WithWriteDefault(std::move(literal));
+ }
Review Comment:
Agreed — `FieldFromJson` now parses the defaults first and builds the field
in one construction. Intermediate copy gone.
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +58,86 @@ 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_);
+}
+
+SchemaField SchemaField::WithInitialDefault(Literal initial_default) const {
+ SchemaField copy = *this;
+ copy.initial_default_ = std::make_shared<const
Literal>(std::move(initial_default));
+ return copy;
+}
Review Comment:
Agreed — moved defaults into the constructor and removed both `With...`
methods, so construction no longer copies the field.
--
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]
