Skip to content

Commit 41c2bd1

Browse files
huan233uschuan233usc
committed
refactor(schema): consolidate default getters and drop [[nodiscard]]
Per review: replace the four default-value getters (optional<reference_wrapper> readers + _ptr sharers) with a single shared_ptr-returning getter per default, and remove the [[nodiscard]] attributes from SchemaField's getters.
1 parent c58050f commit 41c2bd1

7 files changed

Lines changed: 44 additions & 80 deletions

File tree

‎src/iceberg/json_serde.cc‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -327,12 +327,11 @@ Result<nlohmann::json> ToJson(const SchemaField& field) {
327327
if (!field.doc().empty()) {
328328
json[kDoc] = field.doc();
329329
}
330-
if (field.initial_default().has_value()) {
331-
ICEBERG_ASSIGN_OR_RAISE(json[kInitialDefault],
332-
ToJson(field.initial_default()->get()));
330+
if (field.initial_default() != nullptr) {
331+
ICEBERG_ASSIGN_OR_RAISE(json[kInitialDefault], ToJson(*field.initial_default()));
333332
}
334-
if (field.write_default().has_value()) {
335-
ICEBERG_ASSIGN_OR_RAISE(json[kWriteDefault], ToJson(field.write_default()->get()));
333+
if (field.write_default() != nullptr) {
334+
ICEBERG_ASSIGN_OR_RAISE(json[kWriteDefault], ToJson(*field.write_default()));
336335
}
337336
return json;
338337
}

‎src/iceberg/schema.cc‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ SchemaField ReassignField(const SchemaField& field, int32_t new_id,
123123
ReassignTypeIds(field.type(), get_id, ids_to_reassigned, ids_to_original),
124124
field.optional(),
125125
std::string(field.doc()),
126-
field.initial_default_ptr(),
127-
field.write_default_ptr()};
126+
field.initial_default(),
127+
field.write_default()};
128128
}
129129

130130
std::vector<SchemaField> ReassignIds(std::vector<SchemaField> fields,
@@ -457,15 +457,15 @@ Status Schema::Validate(int32_t format_version) const {
457457
// data files are read (rows written before the column existed materialize this
458458
// value), so it requires the v3 reader contract. A write-default only affects
459459
// values written going forward and does not reinterpret existing data.
460-
if (field.initial_default().has_value() &&
460+
if (field.initial_default() != nullptr &&
461461
format_version < TableMetadata::kMinFormatVersionDefaultValues) {
462462
return InvalidSchema(
463463
"Invalid initial default for {}: non-null default ({}) is not supported "
464464
"until v{}",
465-
field.name(), field.initial_default()->get(),
465+
field.name(), *field.initial_default(),
466466
TableMetadata::kMinFormatVersionDefaultValues);
467467
}
468-
if (field.initial_default().has_value() || field.write_default().has_value()) {
468+
if (field.initial_default() != nullptr || field.write_default() != nullptr) {
469469
ICEBERG_RETURN_UNEXPECTED(field.Validate());
470470
}
471471
}

‎src/iceberg/schema_field.cc‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -103,26 +103,11 @@ bool SchemaField::optional() const { return optional_; }
103103

104104
std::string_view SchemaField::doc() const { return doc_; }
105105

106-
std::optional<std::reference_wrapper<const Literal>> SchemaField::initial_default()
107-
const {
108-
if (initial_default_ == nullptr) {
109-
return std::nullopt;
110-
}
111-
return std::cref(*initial_default_);
112-
}
113-
114-
std::optional<std::reference_wrapper<const Literal>> SchemaField::write_default() const {
115-
if (write_default_ == nullptr) {
116-
return std::nullopt;
117-
}
118-
return std::cref(*write_default_);
119-
}
120-
121-
const std::shared_ptr<const Literal>& SchemaField::initial_default_ptr() const {
106+
const std::shared_ptr<const Literal>& SchemaField::initial_default() const {
122107
return initial_default_;
123108
}
124109

125-
const std::shared_ptr<const Literal>& SchemaField::write_default_ptr() const {
110+
const std::shared_ptr<const Literal>& SchemaField::write_default() const {
126111
return write_default_;
127112
}
128113

‎src/iceberg/schema_field.h‎

Lines changed: 12 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,7 @@
2424
/// type (e.g. a struct).
2525

2626
#include <cstdint>
27-
#include <functional>
2827
#include <memory>
29-
#include <optional>
3028
#include <string>
3129
#include <string_view>
3230

@@ -76,47 +74,29 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable {
7674
std::shared_ptr<const Literal> write_default = nullptr);
7775

7876
/// \brief Get the field ID.
79-
[[nodiscard]] int32_t field_id() const;
77+
int32_t field_id() const;
8078

8179
/// \brief Get the field name.
82-
[[nodiscard]] std::string_view name() const;
80+
std::string_view name() const;
8381

8482
/// \brief Get the field type.
85-
[[nodiscard]] const std::shared_ptr<Type>& type() const;
83+
const std::shared_ptr<Type>& type() const;
8684

8785
/// \brief Get whether the field is optional.
88-
[[nodiscard]] bool optional() const;
86+
bool optional() const;
8987

9088
/// \brief Get the field documentation.
9189
std::string_view doc() const;
9290

93-
/// \brief Get the default value for this field used when reading rows written
94-
/// before the field existed (v3 `initial-default`). Empty if absent.
95-
///
96-
/// The returned reference is a non-owning view into a value owned by this field;
97-
/// it remains valid for the lifetime of this SchemaField.
98-
[[nodiscard]] std::optional<std::reference_wrapper<const Literal>> initial_default()
99-
const;
100-
101-
/// \brief Get the default value for this field used when a writer does not
102-
/// supply a value (v3 `write-default`). Empty if absent.
103-
///
104-
/// The returned reference is a non-owning view into a value owned by this field;
105-
/// it remains valid for the lifetime of this SchemaField.
106-
[[nodiscard]] std::optional<std::reference_wrapper<const Literal>> write_default()
107-
const;
108-
109-
/// \brief Get the shared owning pointer to the `initial-default` value, or null if
110-
/// absent. Prefer initial_default() for reading; this exists so a rebuilt field can
111-
/// share the (immutable) value rather than copy it.
112-
[[nodiscard]] const std::shared_ptr<const Literal>& initial_default_ptr() const;
91+
/// \brief Get the owning pointer to the default value for this field used when reading
92+
/// rows written before the field existed (v3 `initial-default`), or null if absent.
93+
const std::shared_ptr<const Literal>& initial_default() const;
11394

114-
/// \brief Get the shared owning pointer to the `write-default` value, or null if
115-
/// absent. Prefer write_default() for reading; this exists so a rebuilt field can
116-
/// share the (immutable) value rather than copy it.
117-
[[nodiscard]] const std::shared_ptr<const Literal>& write_default_ptr() const;
95+
/// \brief Get the owning pointer to the default value for this field used when a writer
96+
/// does not supply a value (v3 `write-default`), or null if absent.
97+
const std::shared_ptr<const Literal>& write_default() const;
11898

119-
[[nodiscard]] std::string ToString() const override;
99+
std::string ToString() const override;
120100

121101
Status Validate() const;
122102

@@ -138,7 +118,7 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable {
138118

139119
private:
140120
/// \brief Compare two fields for equality.
141-
[[nodiscard]] bool Equals(const SchemaField& other) const;
121+
bool Equals(const SchemaField& other) const;
142122

143123
int32_t field_id_;
144124
std::string name_;

‎src/iceberg/schema_util.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,10 +172,10 @@ Result<FieldProjection> ProjectNested(const Type& expected_type, const Type& sou
172172
iter->second.local_index, prune_source));
173173
} else if (MetadataColumns::IsMetadataColumn(field_id)) {
174174
child_projection.kind = FieldProjection::Kind::kMetadata;
175-
} else if (expected_field.initial_default().has_value()) {
175+
} else if (expected_field.initial_default() != nullptr) {
176176
// Rows written before the field existed assume its `initial-default` value.
177177
child_projection.kind = FieldProjection::Kind::kDefault;
178-
child_projection.from = expected_field.initial_default()->get();
178+
child_projection.from = *expected_field.initial_default();
179179
} else if (expected_field.optional()) {
180180
child_projection.kind = FieldProjection::Kind::kNull;
181181
} else {

‎src/iceberg/test/schema_json_test.cc‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -203,15 +203,15 @@ TEST(SchemaJsonTest, FieldWithDefaultValuesRoundTrip) {
203203
ASSERT_EQ(schema->fields().size(), 2);
204204

205205
const auto& field1 = schema->fields()[0];
206-
ASSERT_TRUE(field1.initial_default().has_value());
207-
ASSERT_EQ(field1.initial_default()->get(), Literal::Int(42));
208-
ASSERT_TRUE(field1.write_default().has_value());
209-
ASSERT_EQ(field1.write_default()->get(), Literal::Int(7));
206+
ASSERT_NE(field1.initial_default(), nullptr);
207+
ASSERT_EQ(*field1.initial_default(), Literal::Int(42));
208+
ASSERT_NE(field1.write_default(), nullptr);
209+
ASSERT_EQ(*field1.write_default(), Literal::Int(7));
210210

211211
const auto& field2 = schema->fields()[1];
212-
ASSERT_TRUE(field2.initial_default().has_value());
213-
ASSERT_EQ(field2.initial_default()->get(), Literal::String("n/a"));
214-
ASSERT_FALSE(field2.write_default().has_value());
212+
ASSERT_NE(field2.initial_default(), nullptr);
213+
ASSERT_EQ(*field2.initial_default(), Literal::String("n/a"));
214+
ASSERT_EQ(field2.write_default(), nullptr);
215215

216216
ICEBERG_UNWRAP_OR_FAIL(auto schema_json, ToJson(*schema));
217217
ASSERT_EQ(schema_json.dump(), json);
@@ -232,10 +232,10 @@ TEST(SchemaJsonTest, NestedFieldWithDefaultValuesRoundTrip) {
232232
ICEBERG_UNWRAP_OR_FAIL(auto schema, SchemaFromJson(nlohmann::json::parse(json)));
233233
const auto& person = schema->fields()[0];
234234
const auto& nested = dynamic_cast<const StructType&>(*person.type()).fields()[0];
235-
ASSERT_TRUE(nested.initial_default().has_value());
236-
ASSERT_EQ(nested.initial_default()->get(), Literal::Int(18));
237-
ASSERT_TRUE(nested.write_default().has_value());
238-
ASSERT_EQ(nested.write_default()->get(), Literal::Int(21));
235+
ASSERT_NE(nested.initial_default(), nullptr);
236+
ASSERT_EQ(*nested.initial_default(), Literal::Int(18));
237+
ASSERT_NE(nested.write_default(), nullptr);
238+
ASSERT_EQ(*nested.write_default(), Literal::Int(21));
239239

240240
ICEBERG_UNWRAP_OR_FAIL(auto schema_json, ToJson(*schema));
241241
ASSERT_EQ(schema_json.dump(), json);
@@ -249,8 +249,8 @@ TEST(SchemaJsonTest, CrossTypeDefaultNormalizedRoundTrip) {
249249
ICEBERG_UNWRAP_OR_FAIL(
250250
auto field, SchemaField::Make(1, "id", int64(), /*optional=*/false, /*doc=*/{},
251251
std::make_shared<const Literal>(Literal::Int(34))));
252-
ASSERT_TRUE(field.initial_default().has_value());
253-
EXPECT_EQ(field.initial_default()->get(), Literal::Long(34));
252+
ASSERT_NE(field.initial_default(), nullptr);
253+
EXPECT_EQ(*field.initial_default(), Literal::Long(34));
254254

255255
Schema original({std::move(field)});
256256
ICEBERG_UNWRAP_OR_FAIL(auto json, ToJson(original));

‎src/iceberg/test/schema_test.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,8 @@ TEST(SchemaTest, MakeNormalizesDefaultToFieldType) {
177177
auto field, iceberg::SchemaField::Make(1, "id", iceberg::int64(), false, /*doc=*/{},
178178
std::make_shared<const iceberg::Literal>(
179179
iceberg::Literal::Int(34))));
180-
ASSERT_TRUE(field.initial_default().has_value());
181-
EXPECT_EQ(field.initial_default()->get(), iceberg::Literal::Long(34));
180+
ASSERT_NE(field.initial_default(), nullptr);
181+
EXPECT_EQ(*field.initial_default(), iceberg::Literal::Long(34));
182182

183183
// A default outside the field type's range is rejected by Make.
184184
EXPECT_THAT(iceberg::SchemaField::Make(1, "id", iceberg::int32(), false, /*doc=*/{},
@@ -209,10 +209,10 @@ TEST(SchemaTest, ReassignIdsPreservesDefaultValues) {
209209
ASSERT_EQ(schema.fields().size(), 1);
210210
const iceberg::SchemaField& field = schema.fields()[0];
211211
EXPECT_EQ(field.field_id(), 1001);
212-
ASSERT_TRUE(field.initial_default().has_value());
213-
EXPECT_EQ(field.initial_default()->get(), iceberg::Literal::Int(42));
214-
ASSERT_TRUE(field.write_default().has_value());
215-
EXPECT_EQ(field.write_default()->get(), iceberg::Literal::Int(7));
212+
ASSERT_NE(field.initial_default(), nullptr);
213+
EXPECT_EQ(*field.initial_default(), iceberg::Literal::Int(42));
214+
ASSERT_NE(field.write_default(), nullptr);
215+
EXPECT_EQ(*field.write_default(), iceberg::Literal::Int(7));
216216
}
217217

218218
TEST(SchemaTest, ValidateRejectsInvalidUnknownFields) {

0 commit comments

Comments
 (0)