Skip to content

Commit bda2d78

Browse files
committed
fix(schema): model a null default value as absence
Per review, a present-null default (Literal::Null) is no longer rejected: it is modeled as the absence of a default (matching Java), and dropped at construction so it is never stored. This also makes two such fields compare equal with no change to Literal::operator<=> (no null Literal is ever stored or compared), so the global comparison semantics used by expression predicates are left untouched.
1 parent 41c2bd1 commit bda2d78

2 files changed

Lines changed: 34 additions & 5 deletions

File tree

src/iceberg/schema_field.cc

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,8 @@ Result<std::shared_ptr<const Literal>> NormalizeDefault(
4343
if (value == nullptr || field_type == nullptr || !field_type->is_primitive()) {
4444
return value;
4545
}
46-
if (*value->type() == *field_type) {
46+
if (value->IsNull() || *value->type() == *field_type) {
47+
// A null default is modeled as absence (dropped at construction), so do not cast it.
4748
return value;
4849
}
4950
ICEBERG_ASSIGN_OR_RAISE(
@@ -56,6 +57,15 @@ Result<std::shared_ptr<const Literal>> NormalizeDefault(
5657
return std::make_shared<const Literal>(std::move(cast));
5758
}
5859

60+
// A null default value is modeled as the absence of a default (matching Java), so it is
61+
// not stored.
62+
std::shared_ptr<const Literal> DropNullDefault(std::shared_ptr<const Literal> value) {
63+
if (value != nullptr && value->IsNull()) {
64+
return nullptr;
65+
}
66+
return value;
67+
}
68+
5969
} // namespace
6070

6171
SchemaField::SchemaField(int32_t field_id, std::string_view name,
@@ -67,8 +77,8 @@ SchemaField::SchemaField(int32_t field_id, std::string_view name,
6777
type_(std::move(type)),
6878
optional_(optional),
6979
doc_(doc),
70-
initial_default_(std::move(initial_default)),
71-
write_default_(std::move(write_default)) {}
80+
initial_default_(DropNullDefault(std::move(initial_default))),
81+
write_default_(DropNullDefault(std::move(write_default))) {}
7282

7383
Result<SchemaField> SchemaField::Make(int32_t field_id, std::string_view name,
7484
std::shared_ptr<Type> type, bool optional,
@@ -115,8 +125,10 @@ namespace {
115125

116126
Status ValidateDefault(const SchemaField& field, const Literal& value,
117127
std::string_view kind) {
118-
if (value.IsNull() || value.IsAboveMax() || value.IsBelowMin()) {
119-
return InvalidSchema("Invalid {} value for {}: must be a non-null value", kind,
128+
// A null default is modeled as absence and dropped at construction, so it never reaches
129+
// here; only the out-of-range cast sentinels need rejecting.
130+
if (value.IsAboveMax() || value.IsBelowMin()) {
131+
return InvalidSchema("Invalid {} value for {}: value is out of range", kind,
120132
field.name());
121133
}
122134
// Defaults are only supported on primitive fields. The spec also permits JSON

src/iceberg/test/schema_test.cc

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,23 @@ TEST(SchemaTest, MakeNormalizesDefaultToFieldType) {
193193
iceberg::IsError(iceberg::ErrorKind::kNotSupported));
194194
}
195195

196+
TEST(SchemaTest, NullDefaultModeledAsAbsence) {
197+
// A present-null default is modeled as the absence of a default (matching Java): it is
198+
// dropped at construction, so the field has no stored default and compares equal to a
199+
// field with no default, and it validates cleanly.
200+
iceberg::SchemaField with_null(
201+
1, "id", iceberg::int32(), /*optional=*/true, /*doc=*/{},
202+
std::make_shared<const iceberg::Literal>(iceberg::Literal::Null(iceberg::int32())));
203+
EXPECT_EQ(with_null.initial_default(), nullptr);
204+
205+
iceberg::SchemaField no_default(1, "id", iceberg::int32(), /*optional=*/true);
206+
EXPECT_EQ(with_null, no_default);
207+
208+
iceberg::Schema schema({with_null});
209+
EXPECT_THAT(schema.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
210+
iceberg::IsOk());
211+
}
212+
196213
TEST(SchemaTest, ReassignIdsPreservesDefaultValues) {
197214
// Reassigning field IDs rebuilds each SchemaField, so the rebuild must carry the
198215
// default values over to the field with the new ID.

0 commit comments

Comments
 (0)