You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "huan233usc (via GitHub)" <gi...@apache.org> on 2026/06/15 17:36:03 UTC
[PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
huan233usc opened a new pull request, #746:
URL: https://github.com/apache/iceberg-cpp/pull/746
Part 1 of 4 splitting #730 (column default values, item 2 of #637). The full
end-to-end implementation is in #731, kept open as the proof-of-concept; this series
lands it in reviewable pieces.
This PR is the **schema foundation** — representing, serializing and validating v3
column default values. It changes no read or write behavior on its own.
## What's in this PR
- **`SchemaField`** carries `initial-default` / `write-default`, stored as
`std::shared_ptr<const Literal>` (immutable payload shared across copies, like the
adjacent `type_`; the C++ analog of Java's `final Literal<?>`). Getters return
`std::optional<std::reference_wrapper<const Literal>>` (the `Schema::FindFieldByName`
idiom). Copy-preserving modifiers (`WithInitialDefault` / `WithWriteDefault` /
`WithIdAndType`) ensure reconstruction never silently drops a member.
- **Reconstruction-preservation**: every in-tree site that rebuilds a `SchemaField`
(`ReassignField`, the `type_util` fresh-id / prune visitors, `delete_filter`'s
`MergeField`) routes through `WithIdAndType`.
- **JSON serde**: parse/write `initial-default` / `write-default` using the existing
single-value serialization (all primitive types).
- **`Schema::Validate`**: rejects default values below format v3
(`kMinFormatVersionDefaultValues`), and validates that a default is a non-null
primitive literal matching the field type.
- **Generic projection**: a column missing from a data file with an `initial-default`
maps to `FieldProjection::Kind::kDefault` carrying the literal (the per-format readers
consume this in the follow-up PRs).
## Follow-ups (stacked on this PR)
- read path — Parquet (`literal_util` + parquet projection/materialization)
- read path — Avro
- schema evolution (`UpdateSchema` add/update column defaults)
## Testing
Full suite green. New coverage: schema serde round-trip (incl. nested + mismatch
rejection), `Schema::Validate` v3 gating for both initial- and write-default, type
mismatch rejection, generic projection of missing required/optional fields with
defaults, and id-reassignment preserving defaults. New `TableMetadataV3Valid.json`
test resource.
This pull request and its description were written by Isaac.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
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). This follows Java iiuc
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3484907818
##########
src/iceberg/schema_field.cc:
##########
@@ -72,9 +143,23 @@ std::string SchemaField::ToString() const {
return result;
}
+namespace {
+
+bool DefaultEquals(const std::shared_ptr<const Literal>& lhs,
+ const std::shared_ptr<const Literal>& rhs) {
+ if (lhs == nullptr || rhs == nullptr) {
+ return lhs == rhs;
+ }
+ return *lhs == *rhs;
Review Comment:
Just a reminder that `Literal::operator<=>` returns unordered when any side `IsNull()` returns true so two null defaults do not equal. I think we should fix `Literal::operator<=>` to be null safe?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3484397218
##########
src/iceberg/json_serde.cc:
##########
@@ -307,6 +310,15 @@ nlohmann::json ToJson(const SchemaField& field) {
if (!field.doc().empty()) {
json[kDoc] = field.doc();
}
+ // Defaults are validated to be primitive literals matching the field type, so
+ // single-value serialization cannot fail here.
+ if (field.initial_default().has_value()) {
+ ICEBERG_ASSIGN_OR_THROW(json[kInitialDefault],
Review Comment:
Thanks @wgtmac, this is in general a good call. But the changing the signature to return Result<nlohmann::json> just in case any error requires changing lots of places that is not related on default values.
I updated the pr but also create a pure refactor in https://github.com/apache/iceberg-cpp/pull/785
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3488580982
##########
src/iceberg/util/temporal_util.cc:
##########
@@ -444,6 +444,11 @@ Result<int64_t> TemporalUtils::ParseTimestampNsWithZone(std::string_view str) {
/*units_per_micro=*/internal::kNanosPerMicro);
}
+Result<bool> TemporalUtils::IsUtcOffset(std::string_view str) {
+ ICEBERG_ASSIGN_OR_RAISE(auto timestamp_with_offset, ParseTimestampWithZoneSuffix(str));
Review Comment:
Thanks for raising this — I dug into both the spec and the reference implementation, and I think accepting `Z` / `+00:00` / `-00:00` is correct here.
**What the spec says:** default values use the single-value JSON serialization (Appendix D). For `timestamptz` / `timestamptz_ns` the value must be **UTC**, serialized as an ISO-8601 timestamp with a `+00:00` offset (e.g. `2017-11-16T22:31:08.123456+00:00`). The requirement is that the offset is UTC; `Z` and `-00:00` are just equivalent zero-offset spellings of the same instant.
**What the reference impl does:** `SingleValueParser` gates timestamptz defaults on `DateTimeUtil.isUTCTimestamptz`, which is:
```java
OffsetDateTime odt = OffsetDateTime.parse(s, DateTimeFormatter.ISO_DATE_TIME);
return odt.getOffset().equals(ZoneOffset.UTC);
```
So it checks the parsed offset equals UTC — `Z`, `+00:00` and `-00:00` all pass, and only a genuinely non-zero offset (e.g. `+05:00`) is rejected. That's exactly what `IsUtcOffset` does here. (Java's error string reads "offset must be +00:00", but the underlying check is offset == UTC.)
Refs:
- Spec, Appendix D — Single-value serialization: https://iceberg.apache.org/spec/#appendix-d-single-value-serialization
- `DateTimeUtil.isUTCTimestamptz`: https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java
Happy to tighten it to `+00:00`-only if you'd prefer, though that would reject UTC values the Java reference accepts.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "WZhuo (via GitHub)" <gi...@apache.org>.
WZhuo commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3417952138
##########
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:
I don't think it's need to copy the whole `SchemaField`, can we just set the `initial_default_` field and return *this.
Also the following `With` methods.
##########
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:
`ReassignField` constructs a new `SchemaField` via the 5-argument constructor which initializes `initial_default_` and `write_default_` to `nullptr`. When schema IDs are reassigned (e.g., copying a schema with fresh IDs via the `Schema(get_id)` path), all default values on fields are silently lost. We should copy all field properties including `initialDefault` and `writeDefault`.
##########
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:
The deserialization first constructs a bare `SchemaField`, then conditionally calls `WithInitialDefault`/`WithWriteDefault`, each of which copies the entire field (including the `shared_ptr<Type>`). This is an unnecessary intermediate copy.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3437260655
##########
src/iceberg/schema.cc:
##########
@@ -447,7 +453,21 @@ Status Schema::Validate(int32_t format_version) const {
}
}
- // TODO(GuoTao.yu): Check default values when they are supported
+ // Only the initial-default is gated on format version: it changes how existing
+ // data files are read (rows written before the column existed materialize this
+ // value), so it requires the v3 reader contract. A write-default only affects
+ // values written going forward and does not reinterpret existing data.
+ if (field.initial_default().has_value() &&
+ format_version < TableMetadata::kMinFormatVersionDefaultValues) {
+ return InvalidSchema(
+ "Invalid initial default for {}: non-null default ({}) is not supported "
+ "until v{}",
+ field.name(), field.initial_default()->get(),
+ TableMetadata::kMinFormatVersionDefaultValues);
+ }
+ if (field.initial_default().has_value() || field.write_default().has_value()) {
+ ICEBERG_RETURN_UNEXPECTED(field.Validate());
+ }
Review Comment:
The PR description says `Schema::Validate` rejects default values below format v3, but this implementation only version-gates `initial-default` and explicitly allows `write-default` for format < v3 (and there’s a new test asserting that behavior). Please align the implementation/tests with the intended contract (either gate `write-default` too, or update the PR description/constant naming to reflect the narrower gating).
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,64 @@ 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 {}: {} (must be null)", kind, field.name(),
+ value);
+ }
Review Comment:
`ValidateDefault` returns an error message saying the default "(must be null)" when the field type is non-primitive, but earlier in the same function null defaults are rejected. This message is confusing and doesn’t reflect the real constraint (defaults are only supported for primitive field types).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3489133468
##########
src/iceberg/schema_field.cc:
##########
@@ -21,19 +21,26 @@
#include <format>
#include <string_view>
+#include <utility>
+#include "iceberg/expression/literal.h"
#include "iceberg/type.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
+#include "iceberg/util/macros.h"
namespace iceberg {
Review Comment:
Done, remove the normalization method
##########
src/iceberg/schema_field.cc:
##########
@@ -21,19 +21,26 @@
#include <format>
#include <string_view>
+#include <utility>
+#include "iceberg/expression/literal.h"
#include "iceberg/type.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
+#include "iceberg/util/macros.h"
namespace iceberg {
Review Comment:
Done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3486909101
##########
src/iceberg/util/temporal_util.cc:
##########
@@ -444,6 +444,11 @@ Result<int64_t> TemporalUtils::ParseTimestampNsWithZone(std::string_view str) {
/*units_per_micro=*/internal::kNanosPerMicro);
}
+Result<bool> TemporalUtils::IsUtcOffset(std::string_view str) {
+ ICEBERG_ASSIGN_OR_RAISE(auto timestamp_with_offset, ParseTimestampWithZoneSuffix(str));
Review Comment:
`Z`, `+00:00` and `-00:00` all denote a zero (UTC) offset, so accepting them is consistent with UTC semantics even though the spec writes the canonical form as `+00:00` (Java's `SingleValueParser` accepts the equivalents too). Documented this explicitly on `TemporalUtils::IsUtcOffset` in 26b1f1b.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3447209143
##########
src/iceberg/test/resources/TableMetadataV3Valid.json:
##########
@@ -0,0 +1,123 @@
+{
Review Comment:
Good catch — it was unused. Removed in 511d9c8.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3482324864
##########
src/iceberg/schema_field.cc:
##########
@@ -72,9 +142,23 @@ std::string SchemaField::ToString() const {
return result;
}
+namespace {
+
+bool DefaultEquals(const std::shared_ptr<const Literal>& lhs,
+ const std::shared_ptr<const Literal>& rhs) {
+ if (lhs == nullptr || rhs == nullptr) {
+ return lhs == rhs;
+ }
Review Comment:
Should we fix this in `Literal::operator<=>(const Literal& other)`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#issuecomment-4775532988
Thank you @zhjwpku and @WZhuo for review!
Hi @wgtmac, do you mind take another look as you requested copilot to review on your behalf. Thanks
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3437718784
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,64 @@ 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 {}: {} (must be null)", kind, field.name(),
+ value);
+ }
Review Comment:
Fixed — the non-primitive case now reports "default values are only supported for primitive types" instead of the misleading "(must be null)".
##########
src/iceberg/schema.cc:
##########
@@ -447,7 +453,21 @@ Status Schema::Validate(int32_t format_version) const {
}
}
- // TODO(GuoTao.yu): Check default values when they are supported
+ // Only the initial-default is gated on format version: it changes how existing
+ // data files are read (rows written before the column existed materialize this
+ // value), so it requires the v3 reader contract. A write-default only affects
+ // values written going forward and does not reinterpret existing data.
+ if (field.initial_default().has_value() &&
+ format_version < TableMetadata::kMinFormatVersionDefaultValues) {
+ return InvalidSchema(
+ "Invalid initial default for {}: non-null default ({}) is not supported "
+ "until v{}",
+ field.name(), field.initial_default()->get(),
+ TableMetadata::kMinFormatVersionDefaultValues);
+ }
+ if (field.initial_default().has_value() || field.write_default().has_value()) {
+ ICEBERG_RETURN_UNEXPECTED(field.Validate());
+ }
Review Comment:
The narrower gating is intentional: only `initial-default` is version-gated (it reinterprets how existing data is read), while `write-default` is allowed below v3 — matching Java's `Schema.checkCompatibility`, which gates only the initial default. Updated the PR description to state this explicitly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3484901838
##########
src/iceberg/json_serde.cc:
##########
@@ -307,6 +310,15 @@ nlohmann::json ToJson(const SchemaField& field) {
if (!field.doc().empty()) {
json[kDoc] = field.doc();
}
+ // Defaults are validated to be primitive literals matching the field type, so
+ // single-value serialization cannot fail here.
+ if (field.initial_default().has_value()) {
+ ICEBERG_ASSIGN_OR_THROW(json[kInitialDefault],
Review Comment:
Thanks! Merged that PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3445310006
##########
src/iceberg/json_serde.cc:
##########
@@ -307,6 +300,15 @@ nlohmann::json ToJson(const SchemaField& field) {
if (!field.doc().empty()) {
json[kDoc] = field.doc();
}
+ // Defaults are validated to be primitive literals matching the field type, so
Review Comment:
Should we add a round-trip serde test for default values?
##########
src/iceberg/test/resources/TableMetadataV3Valid.json:
##########
@@ -0,0 +1,123 @@
+{
Review Comment:
This metadata file seems not being used anywhere.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3447207901
##########
src/iceberg/json_serde.cc:
##########
@@ -307,6 +300,15 @@ nlohmann::json ToJson(const SchemaField& field) {
if (!field.doc().empty()) {
json[kDoc] = field.doc();
}
+ // Defaults are validated to be primitive literals matching the field type, so
Review Comment:
Added default-value serde round-trip tests in `json_serde_test.cc` (9690529), covering all primitive types.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3486909101
##########
src/iceberg/util/temporal_util.cc:
##########
@@ -444,6 +444,11 @@ Result<int64_t> TemporalUtils::ParseTimestampNsWithZone(std::string_view str) {
/*units_per_micro=*/internal::kNanosPerMicro);
}
+Result<bool> TemporalUtils::IsUtcOffset(std::string_view str) {
+ ICEBERG_ASSIGN_OR_RAISE(auto timestamp_with_offset, ParseTimestampWithZoneSuffix(str));
Review Comment:
`Z`, `+00:00` and `-00:00` all denote a zero (UTC) offset, so accepting them is consistent with UTC semantics even though the spec writes the canonical form as `+00:00` (Java's `SingleValueParser` accepts the equivalents too). Documented this explicitly on `TemporalUtils::IsUtcOffset` in 26b1f1b.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3488580982
##########
src/iceberg/util/temporal_util.cc:
##########
@@ -444,6 +444,11 @@ Result<int64_t> TemporalUtils::ParseTimestampNsWithZone(std::string_view str) {
/*units_per_micro=*/internal::kNanosPerMicro);
}
+Result<bool> TemporalUtils::IsUtcOffset(std::string_view str) {
+ ICEBERG_ASSIGN_OR_RAISE(auto timestamp_with_offset, ParseTimestampWithZoneSuffix(str));
Review Comment:
To close this definitively: the Java reference accepts `Z`, `+00:00` and `-00:00` too. `SingleValueParser` gates timestamptz defaults on `DateTimeUtil.isUTCTimestamptz`, which is:
```java
OffsetDateTime offsetDateTime = OffsetDateTime.parse(s, DateTimeFormatter.ISO_DATE_TIME);
return offsetDateTime.getOffset().equals(ZoneOffset.UTC);
```
i.e. it checks the parsed offset equals UTC (zero), so `Z`/`+00:00`/`-00:00` all pass and only a non-zero offset is rejected — exactly what `IsUtcOffset` does here. (Java's error string says "offset must be +00:00", but the actual check is offset == UTC.)
Ref: https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3482238911
##########
src/iceberg/schema_field.h:
##########
@@ -71,6 +79,32 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable {
/// \brief Get the field documentation.
std::string_view doc() const;
+ /// \brief Get the default value for this field used when reading rows written
+ /// before the field existed (v3 `initial-default`). Empty if absent.
+ ///
+ /// The returned reference is a non-owning view into a value owned by this field;
+ /// it remains valid for the lifetime of this SchemaField.
+ [[nodiscard]] std::optional<std::reference_wrapper<const Literal>> initial_default()
+ const;
+
+ /// \brief Get the default value for this field used when a writer does not
+ /// supply a value (v3 `write-default`). Empty if absent.
+ ///
+ /// The returned reference is a non-owning view into a value owned by this field;
+ /// it remains valid for the lifetime of this SchemaField.
+ [[nodiscard]] std::optional<std::reference_wrapper<const Literal>> write_default()
+ const;
+
+ /// \brief Get the shared owning pointer to the `initial-default` value, or null if
+ /// absent. Prefer initial_default() for reading; this exists so a rebuilt field can
+ /// share the (immutable) value rather than copy it.
+ [[nodiscard]] const std::shared_ptr<const Literal>& initial_default_ptr() const;
Review Comment:
It looks odd to provide both `initial_default` and `initial_default_ptr`. Should we keep one, like `const std::shared_ptr<const Literal>& initial_default() const`?
##########
src/iceberg/schema_field.h:
##########
@@ -100,6 +134,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
Review Comment:
Not a strong opinion but the comment looks too verbose.
##########
src/iceberg/schema_field.h:
##########
@@ -71,6 +79,32 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable {
/// \brief Get the field documentation.
std::string_view doc() const;
+ /// \brief Get the default value for this field used when reading rows written
+ /// before the field existed (v3 `initial-default`). Empty if absent.
+ ///
+ /// The returned reference is a non-owning view into a value owned by this field;
+ /// it remains valid for the lifetime of this SchemaField.
+ [[nodiscard]] std::optional<std::reference_wrapper<const Literal>> initial_default()
Review Comment:
I think we should remove `[[nodiscard]]` because users should always know what they are doing when these are called. I think we need also clean up existing functions as well.
##########
src/iceberg/schema_field.cc:
##########
@@ -72,9 +142,23 @@ std::string SchemaField::ToString() const {
return result;
}
+namespace {
+
+bool DefaultEquals(const std::shared_ptr<const Literal>& lhs,
+ const std::shared_ptr<const Literal>& rhs) {
+ if (lhs == nullptr || rhs == nullptr) {
+ return lhs == rhs;
+ }
Review Comment:
Should we fix this in `Literal::operator<=>(const Literal& other)`?
##########
src/iceberg/schema_field.cc:
##########
@@ -21,19 +21,26 @@
#include <format>
#include <string_view>
+#include <utility>
+#include "iceberg/expression/literal.h"
#include "iceberg/type.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
+#include "iceberg/util/macros.h"
namespace iceberg {
Review Comment:
This always create a new literal even when their types are the same which is unlikely?
##########
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);
+ if (!cast.has_value() || cast->IsAboveMax() || cast->IsBelowMin()) {
Review Comment:
Perhaps it is better to use `ICEBERG_ASSIGN_OR_RAISE` to respect the original error instead of checking `cast.has_value()`?
##########
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());
Review Comment:
nit: use `internal::checked_pointer_cast`
##########
src/iceberg/json_serde.cc:
##########
@@ -554,16 +565,67 @@ Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
}
}
+namespace {
+
+// The spec's JSON single-value form for `timestamptz` / `timestamptz_ns` default
+// values requires a UTC offset. The shared timestamp parser accepts any offset and
+// silently normalizes to UTC, which would let C++ accept default metadata that Java
+// rejects and then rewrite the offset on serialization. Enforce UTC for these
+// defaults at parse time, where the original offset is still visible.
+Status ValidateTimestamptzDefaultIsUtc(const Type& type, const nlohmann::json& value) {
+ const auto type_id = type.type_id();
+ if (type_id != TypeId::kTimestampTz && type_id != TypeId::kTimestampTzNs) {
+ return {};
+ }
+ if (!value.is_string()) {
+ // Let LiteralFromJson report the type mismatch.
+ return {};
+ }
+ const auto str = value.get<std::string>();
+ ICEBERG_ASSIGN_OR_RAISE(bool is_utc, TemporalUtils::IsUtcOffset(str));
+ if (!is_utc) {
+ return JsonParseError(
+ "Invalid timestamptz default '{}' for {}: default values must use UTC "
+ "(offset 'Z' or '+00:00')",
Review Comment:
As I have commented earlier, this can also accept `-00:00` so the error message is not consistent.
##########
src/iceberg/json_serde.cc:
##########
@@ -554,16 +565,67 @@ Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
}
}
+namespace {
+
+// The spec's JSON single-value form for `timestamptz` / `timestamptz_ns` default
+// values requires a UTC offset. The shared timestamp parser accepts any offset and
+// silently normalizes to UTC, which would let C++ accept default metadata that Java
+// rejects and then rewrite the offset on serialization. Enforce UTC for these
+// defaults at parse time, where the original offset is still visible.
+Status ValidateTimestamptzDefaultIsUtc(const Type& type, const nlohmann::json& value) {
+ const auto type_id = type.type_id();
+ if (type_id != TypeId::kTimestampTz && type_id != TypeId::kTimestampTzNs) {
+ return {};
+ }
+ if (!value.is_string()) {
+ // Let LiteralFromJson report the type mismatch.
+ return {};
+ }
+ const auto str = value.get<std::string>();
+ ICEBERG_ASSIGN_OR_RAISE(bool is_utc, TemporalUtils::IsUtcOffset(str));
+ if (!is_utc) {
+ return JsonParseError(
+ "Invalid timestamptz default '{}' for {}: default values must use UTC "
+ "(offset 'Z' or '+00:00')",
+ str, type.ToString());
+ }
+ return {};
+}
+
+} // namespace
+
Result<std::unique_ptr<SchemaField>> FieldFromJson(const nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(
auto type, GetJsonValue<nlohmann::json>(json, kType).and_then(TypeFromJson));
ICEBERG_ASSIGN_OR_RAISE(auto field_id, GetJsonValue<int32_t>(json, kId));
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,
Review Comment:
Why not use `auto`?
##########
src/iceberg/util/temporal_util.cc:
##########
@@ -444,6 +444,11 @@ Result<int64_t> TemporalUtils::ParseTimestampNsWithZone(std::string_view str) {
/*units_per_micro=*/internal::kNanosPerMicro);
}
+Result<bool> TemporalUtils::IsUtcOffset(std::string_view str) {
+ ICEBERG_ASSIGN_OR_RAISE(auto timestamp_with_offset, ParseTimestampWithZoneSuffix(str));
Review Comment:
This allows `Z` and `-00:00` as well. This is not allowed by the spec, right? However, Java impl seems doing the same thing. Perhaps we need to document this behavior explicitly?
##########
src/iceberg/schema_field.cc:
##########
@@ -21,19 +21,26 @@
#include <format>
#include <string_view>
+#include <utility>
+#include "iceberg/expression/literal.h"
#include "iceberg/type.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
+#include "iceberg/util/macros.h"
namespace iceberg {
Review Comment:
Should we return `Result<std::shared_ptr<const Literal>>` so we don't swallow any unexpected error?
##########
src/iceberg/json_serde.cc:
##########
@@ -307,6 +310,15 @@ nlohmann::json ToJson(const SchemaField& field) {
if (!field.doc().empty()) {
json[kDoc] = field.doc();
}
+ // Defaults are validated to be primitive literals matching the field type, so
+ // single-value serialization cannot fail here.
+ if (field.initial_default().has_value()) {
+ ICEBERG_ASSIGN_OR_THROW(json[kInitialDefault],
Review Comment:
Though it cannot not fail, we shouldn't use `ICEBERG_ASSIGN_OR_THROW` here. Let's change the signature to return `Result<nlohmann::json>` just in case any error.
##########
src/iceberg/json_serde.cc:
##########
@@ -554,16 +565,67 @@ Result<std::unique_ptr<Type>> TypeFromJson(const nlohmann::json& json) {
}
}
+namespace {
+
+// The spec's JSON single-value form for `timestamptz` / `timestamptz_ns` default
+// values requires a UTC offset. The shared timestamp parser accepts any offset and
+// silently normalizes to UTC, which would let C++ accept default metadata that Java
+// rejects and then rewrite the offset on serialization. Enforce UTC for these
+// defaults at parse time, where the original offset is still visible.
+Status ValidateTimestamptzDefaultIsUtc(const Type& type, const nlohmann::json& value) {
+ const auto type_id = type.type_id();
+ if (type_id != TypeId::kTimestampTz && type_id != TypeId::kTimestampTzNs) {
+ return {};
+ }
+ if (!value.is_string()) {
+ // Let LiteralFromJson report the type mismatch.
Review Comment:
Why not directly error out here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3484909229
##########
src/iceberg/schema_field.cc:
##########
@@ -72,9 +143,23 @@ std::string SchemaField::ToString() const {
return result;
}
+namespace {
+
+bool DefaultEquals(const std::shared_ptr<const Literal>& lhs,
+ const std::shared_ptr<const Literal>& rhs) {
+ if (lhs == nullptr || rhs == nullptr) {
+ return lhs == rhs;
+ }
+ return *lhs == *rhs;
Review Comment:
Or add a overload `Literal::NullSafeEquals()`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "huan233usc (via GitHub)" <gi...@apache.org>.
huan233usc commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3488580982
##########
src/iceberg/util/temporal_util.cc:
##########
@@ -444,6 +444,11 @@ Result<int64_t> TemporalUtils::ParseTimestampNsWithZone(std::string_view str) {
/*units_per_micro=*/internal::kNanosPerMicro);
}
+Result<bool> TemporalUtils::IsUtcOffset(std::string_view str) {
+ ICEBERG_ASSIGN_OR_RAISE(auto timestamp_with_offset, ParseTimestampWithZoneSuffix(str));
Review Comment:
It seems that Java reference accepts `Z`, `+00:00` and `-00:00` too. `SingleValueParser` gates timestamptz defaults on `DateTimeUtil.isUTCTimestamptz`, which is:
```java
OffsetDateTime offsetDateTime = OffsetDateTime.parse(s, DateTimeFormatter.ISO_DATE_TIME);
return offsetDateTime.getOffset().equals(ZoneOffset.UTC);
```
i.e. it checks the parsed offset equals UTC (zero), so `Z`/`+00:00`/`-00:00` all pass and only a non-zero offset is rejected — exactly what `IsUtcOffset` does here. (Java's error string says "offset must be +00:00", but the actual check is offset == UTC.)
Ref: https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(schema): represent, serialize and validate v3 column default values (1/4) [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org