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: [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]
