wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3478339282
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,76 @@ 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());
+ }
+ // Defaults are only supported on primitive fields. The spec also permits
JSON
+ // single-value defaults for struct/list/map (e.g. an empty struct `{}` whose
+ // sub-field defaults live in field metadata); that matches the current Java
model's
+ // gap and is left as a follow-up.
+ if (field.type() == nullptr || !field.type()->is_primitive()) {
+ return InvalidSchema(
+ "Invalid {} value for {}: default values are only supported for
primitive types",
+ kind, field.name());
+ }
+ // Match Java (Types.NestedField), which casts the default literal to the
field type
+ // instead of requiring an exact type match (e.g. an int default on a long
field, or
+ // a string default on a date/timestamp/uuid field). Reject only defaults
that cannot
+ // be cast to the field type or fall outside its range (CastTo signals
out-of-range as
+ // an above-max/below-min sentinel).
+ auto field_type = std::static_pointer_cast<PrimitiveType>(field.type());
+ auto cast = value.CastTo(field_type);
Review Comment:
This validates the cast but keeps storing the original literal. Java stores
the result of `castDefault`, so a programmatic default like
`Literal::Long(...)` on a timestamp field would validate here, then later
serialize or project as a long instead of a timestamp default. Either the field
needs to retain the casted literal, or this should keep requiring an exact type
match.
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,76 @@ 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()) {
Review Comment:
This rejects explicit null defaults. The spec allows optional field defaults
to be null and says they may be explicitly set. Java mostly models a null
default as absence, but C++ now has a present-null `Literal` path that will
fail validation and, for `initial-default`, also trips the format-version gate
by presence rather than by non-null value.
--
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]
