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