Skip to content

Commit d022953

Browse files
committed
refactor(schema): drop unused default-value cast machinery
SchemaField::Make and the file-local NormalizeDefault cast a cross-type default (e.g. an int literal on a long field) to the field type, but they have no production callers: defaults arrive already at the field type (FieldFromJson parses with the field type), and ValidateDefault's exact type match already rejects any cross-type default. The cast was therefore dead in production and, as wgtmac noted in review, allocated a new Literal even in the same-type case. Remove Make and NormalizeDefault (and their tests). The constructor stores defaults verbatim and Validate rejects type mismatches. The cast belongs in the follow-up that adds a real caller (e.g. schema evolution), where Java performs it in the NestedField constructor (castDefault).
1 parent 70e029b commit d022953

4 files changed

Lines changed: 3 additions & 94 deletions

File tree

src/iceberg/schema_field.cc

Lines changed: 2 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -25,38 +25,13 @@
2525

2626
#include "iceberg/expression/literal.h"
2727
#include "iceberg/type.h"
28-
#include "iceberg/util/checked_cast.h"
2928
#include "iceberg/util/formatter.h" // IWYU pragma: keep
3029
#include "iceberg/util/macros.h"
3130

3231
namespace iceberg {
3332

3433
namespace {
3534

36-
// Normalizes a default value to the field type so the stored value always matches the
37-
// field. A cross-type default (e.g. an int literal on a long field) is cast to the field
38-
// type; a value that already matches the field type, or a null, is returned unchanged.
39-
// The cast error is propagated and an out-of-range value is rejected rather than
40-
// swallowed.
41-
Result<std::shared_ptr<const Literal>> NormalizeDefault(
42-
std::shared_ptr<const Literal> value, const std::shared_ptr<Type>& field_type) {
43-
if (value == nullptr || field_type == nullptr || !field_type->is_primitive()) {
44-
return value;
45-
}
46-
if (value->IsNull() || *value->type() == *field_type) {
47-
// A null default is modeled as absence (dropped at construction), so do not cast it.
48-
return value;
49-
}
50-
ICEBERG_ASSIGN_OR_RAISE(
51-
auto cast,
52-
value->CastTo(internal::checked_pointer_cast<PrimitiveType>(field_type)));
53-
if (cast.IsAboveMax() || cast.IsBelowMin()) {
54-
return InvalidSchema("default value of type {} is out of range for {}",
55-
*value->type(), *field_type);
56-
}
57-
return std::make_shared<const Literal>(std::move(cast));
58-
}
59-
6035
// A null default value is modeled as the absence of a default (matching Java), so it is
6136
// not stored.
6237
std::shared_ptr<const Literal> DropNullDefault(std::shared_ptr<const Literal> value) {
@@ -80,19 +55,6 @@ SchemaField::SchemaField(int32_t field_id, std::string_view name,
8055
initial_default_(DropNullDefault(std::move(initial_default))),
8156
write_default_(DropNullDefault(std::move(write_default))) {}
8257

83-
Result<SchemaField> SchemaField::Make(int32_t field_id, std::string_view name,
84-
std::shared_ptr<Type> type, bool optional,
85-
std::string_view doc,
86-
std::shared_ptr<const Literal> initial_default,
87-
std::shared_ptr<const Literal> write_default) {
88-
ICEBERG_ASSIGN_OR_RAISE(initial_default,
89-
NormalizeDefault(std::move(initial_default), type));
90-
ICEBERG_ASSIGN_OR_RAISE(write_default,
91-
NormalizeDefault(std::move(write_default), type));
92-
return SchemaField(field_id, name, std::move(type), optional, doc,
93-
std::move(initial_default), std::move(write_default));
94-
}
95-
9658
SchemaField SchemaField::MakeOptional(int32_t field_id, std::string_view name,
9759
std::shared_ptr<Type> type, std::string_view doc) {
9860
return {field_id, name, std::move(type), true, doc};
@@ -156,9 +118,8 @@ Status ValidateDefault(const SchemaField& field, const Literal& value,
156118
"Invalid {} value for {}: default values are only supported for primitive types",
157119
kind, field.name());
158120
}
159-
// A default is normalized to the field type by SchemaField::Make (which casts a
160-
// cross-type default and reports any cast error), so a stored default whose type does
161-
// not match the field type is invalid here.
121+
// Defaults are stored verbatim (no implicit cast), so a default whose literal type does
122+
// not match the field type is invalid.
162123
if (*value.type() != *field.type()) {
163124
return InvalidSchema("{} of field {} has type {} but expected {}", kind, field.name(),
164125
*value.type(), *field.type());

src/iceberg/schema_field.h

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,6 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable {
6262
static SchemaField MakeRequired(int32_t field_id, std::string_view name,
6363
std::shared_ptr<Type> type, std::string_view doc = {});
6464

65-
/// \brief Construct a field, normalizing the default values to the field type.
66-
///
67-
/// Unlike the constructor (which stores the defaults verbatim), this casts a default
68-
/// whose literal type differs from the field type to the field type — e.g. an int
69-
/// default on a long field — matching the spec/Java cast behavior. Returns an error if
70-
/// a default cannot be cast to the field type or falls outside its range.
71-
static Result<SchemaField> Make(
72-
int32_t field_id, std::string_view name, std::shared_ptr<Type> type, bool optional,
73-
std::string_view doc = {}, std::shared_ptr<const Literal> initial_default = nullptr,
74-
std::shared_ptr<const Literal> write_default = nullptr);
75-
7665
/// \brief Get the field ID.
7766
[[nodiscard]] int32_t field_id() const;
7867

src/iceberg/test/schema_json_test.cc

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -241,23 +241,6 @@ TEST(SchemaJsonTest, NestedFieldWithDefaultValuesRoundTrip) {
241241
ASSERT_EQ(schema_json.dump(), json);
242242
}
243243

244-
TEST(SchemaJsonTest, CrossTypeDefaultNormalizedRoundTrip) {
245-
// A cross-type default (an int literal on a long field) built via SchemaField::Make is
246-
// normalized to the field type, so it survives a ToJson/SchemaFromJson round-trip as an
247-
// equal schema (an un-normalized int literal would compare unequal to the re-parsed
248-
// long literal).
249-
ICEBERG_UNWRAP_OR_FAIL(
250-
auto field, SchemaField::Make(1, "id", int64(), /*optional=*/false, /*doc=*/{},
251-
std::make_shared<const Literal>(Literal::Int(34))));
252-
ASSERT_NE(field.initial_default(), nullptr);
253-
EXPECT_EQ(*field.initial_default(), Literal::Long(34));
254-
255-
Schema original({std::move(field)});
256-
ICEBERG_UNWRAP_OR_FAIL(auto json, ToJson(original));
257-
ICEBERG_UNWRAP_OR_FAIL(auto reparsed, SchemaFromJson(json));
258-
EXPECT_EQ(original, *reparsed);
259-
}
260-
261244
TEST(SchemaJsonTest, UnknownFieldRoundTrip) {
262245
constexpr std::string_view json =
263246
R"({"fields":[{"id":1,"name":"mystery","required":false,"type":"unknown"}],"schema-id":1,"type":"struct"})";

src/iceberg/test/schema_test.cc

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,7 @@ TEST(SchemaTest, ValidateDoesNotVersionGateWriteDefault) {
158158
}
159159

160160
TEST(SchemaTest, ValidateRejectsMismatchedDefaultValue) {
161-
// The constructor stores defaults verbatim (normalization happens in
162-
// SchemaField::Make), so a stored default whose type differs from the field type is
161+
// Defaults are stored verbatim, so a default whose type differs from the field type is
163162
// rejected by Validate.
164163
iceberg::Schema schema({iceberg::SchemaField(
165164
1, "id", iceberg::int32(), false, /*doc=*/{}, /*initial_default=*/nullptr,
@@ -170,29 +169,6 @@ TEST(SchemaTest, ValidateRejectsMismatchedDefaultValue) {
170169
EXPECT_THAT(status, iceberg::HasErrorMessage("write-default"));
171170
}
172171

173-
TEST(SchemaTest, MakeNormalizesDefaultToFieldType) {
174-
// Make casts a cross-type default to the field type (int -> long) and stores the
175-
// normalized value, so projection/serialization/equality all observe a long.
176-
ICEBERG_UNWRAP_OR_FAIL(
177-
auto field, iceberg::SchemaField::Make(1, "id", iceberg::int64(), false, /*doc=*/{},
178-
std::make_shared<const iceberg::Literal>(
179-
iceberg::Literal::Int(34))));
180-
ASSERT_NE(field.initial_default(), nullptr);
181-
EXPECT_EQ(*field.initial_default(), iceberg::Literal::Long(34));
182-
183-
// A default outside the field type's range is rejected by Make.
184-
EXPECT_THAT(iceberg::SchemaField::Make(1, "id", iceberg::int32(), false, /*doc=*/{},
185-
std::make_shared<const iceberg::Literal>(
186-
iceberg::Literal::Long(5'000'000'000))),
187-
iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
188-
189-
// A default whose type cannot be cast to the field type surfaces the cast error.
190-
EXPECT_THAT(iceberg::SchemaField::Make(1, "id", iceberg::int32(), false, /*doc=*/{},
191-
std::make_shared<const iceberg::Literal>(
192-
iceberg::Literal::String("oops"))),
193-
iceberg::IsError(iceberg::ErrorKind::kNotSupported));
194-
}
195-
196172
TEST(SchemaTest, NullDefaultModeledAsAbsence) {
197173
// A present-null default is modeled as the absence of a default (matching Java): it is
198174
// dropped at construction, so the field has no stored default and compares equal to a

0 commit comments

Comments
 (0)