Skip to content

Commit 1ca6c61

Browse files
zhjwpkuzhjwpku
committed
fix: change format version back to 1/2/3
1 parent 8a10475 commit 1ca6c61

14 files changed

Lines changed: 86 additions & 113 deletions

‎src/iceberg/constants.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,6 @@
3030

3131
namespace iceberg {
3232

33-
/// \brief Iceberg table format versions
34-
constexpr int8_t kFormatVersion1 = 1;
35-
constexpr int8_t kFormatVersion2 = 2;
36-
constexpr int8_t kFormatVersion3 = 3;
37-
3833
constexpr std::string_view kParquetFieldIdKey = "PARQUET:field_id";
3934
constexpr int64_t kInvalidSnapshotId = -1;
4035
constexpr int64_t kInvalidSequenceNumber = -1;

‎src/iceberg/json_serde.cc‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
838838
json[kFormatVersion] = table_metadata.format_version;
839839
json[kTableUuid] = table_metadata.table_uuid;
840840
json[kLocation] = table_metadata.location;
841-
if (table_metadata.format_version > kFormatVersion1) {
841+
if (table_metadata.format_version > 1) {
842842
json[kLastSequenceNumber] = table_metadata.last_sequence_number;
843843
}
844844
json[kLastUpdatedMs] = UnixMsFromTimePointMs(table_metadata.last_updated_ms);
@@ -847,7 +847,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
847847
// for older readers, continue writing the current schema as "schema".
848848
// this is only needed for v1 because support for schemas and current-schema-id
849849
// is required in v2 and later.
850-
if (table_metadata.format_version == kFormatVersion1) {
850+
if (table_metadata.format_version == 1) {
851851
for (const auto& schema : table_metadata.schemas) {
852852
if (schema->schema_id() == table_metadata.current_schema_id) {
853853
json[kSchema] = ToJson(*schema);
@@ -861,7 +861,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
861861
json[kSchemas] = ToJsonList(table_metadata.schemas);
862862

863863
// for older readers, continue writing the default spec as "partition-spec"
864-
if (table_metadata.format_version == kFormatVersion1) {
864+
if (table_metadata.format_version == 1) {
865865
for (const auto& partition_spec : table_metadata.partition_specs) {
866866
if (partition_spec->spec_id() == table_metadata.default_spec_id) {
867867
json[kPartitionSpec] = ToJson(*partition_spec);
@@ -890,7 +890,7 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) {
890890
json[kCurrentSnapshotId] = nlohmann::json::value_t::null;
891891
}
892892

893-
if (table_metadata.format_version >= kFormatVersion3) {
893+
if (table_metadata.format_version >= 3) {
894894
json[kNextRowId] = table_metadata.next_row_id;
895895
}
896896

@@ -945,7 +945,7 @@ Result<std::shared_ptr<Schema>> ParseSchemas(
945945
current_schema_id, SafeDumpJson(schema_array));
946946
}
947947
} else {
948-
if (format_version != kFormatVersion1) {
948+
if (format_version != 1) {
949949
return JsonParseError("{} must exist in format v{}", kSchemas, format_version);
950950
}
951951
ICEBERG_ASSIGN_OR_RAISE(auto schema_json,
@@ -983,7 +983,7 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
983983
partition_specs.push_back(std::move(spec));
984984
}
985985
} else {
986-
if (format_version != kFormatVersion1) {
986+
if (format_version != 1) {
987987
return JsonParseError("{} must exist in format v{}", kPartitionSpecs,
988988
format_version);
989989
}
@@ -999,9 +999,8 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
999999
std::vector<PartitionField> fields;
10001000
for (const auto& entry_json : partition_spec_json) {
10011001
ICEBERG_ASSIGN_OR_RAISE(
1002-
auto field,
1003-
PartitionFieldFromJson(
1004-
entry_json, /*allow_field_id_missing=*/format_version == kFormatVersion1));
1002+
auto field, PartitionFieldFromJson(
1003+
entry_json, /*allow_field_id_missing=*/format_version == 1));
10051004
int32_t field_id = field->field_id();
10061005
if (field_id == SchemaField::kInvalidFieldId) {
10071006
// If the field ID is not set, we need to assign a new one
@@ -1045,7 +1044,7 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version,
10451044
sort_orders.push_back(std::move(sort_order));
10461045
}
10471046
} else {
1048-
if (format_version > kFormatVersion1) {
1047+
if (format_version > 1) {
10491048
return JsonParseError("{} must exist in format v{}", kSortOrders, format_version);
10501049
}
10511050
auto sort_order = SortOrder::Unsorted();
@@ -1067,7 +1066,7 @@ Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::jso
10671066

10681067
ICEBERG_ASSIGN_OR_RAISE(table_metadata->format_version,
10691068
GetJsonValue<int8_t>(json, kFormatVersion));
1070-
if (table_metadata->format_version < kFormatVersion1 ||
1069+
if (table_metadata->format_version < 1 ||
10711070
table_metadata->format_version > TableMetadata::kSupportedTableFormatVersion) {
10721071
return JsonParseError("Cannot read unsupported version: {}",
10731072
table_metadata->format_version);
@@ -1078,7 +1077,7 @@ Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::jso
10781077
ICEBERG_ASSIGN_OR_RAISE(table_metadata->location,
10791078
GetJsonValue<std::string>(json, kLocation));
10801079

1081-
if (table_metadata->format_version > kFormatVersion1) {
1080+
if (table_metadata->format_version > 1) {
10821081
ICEBERG_ASSIGN_OR_RAISE(table_metadata->last_sequence_number,
10831082
GetJsonValue<int64_t>(json, kLastSequenceNumber));
10841083
} else {
@@ -1100,7 +1099,7 @@ Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::jso
11001099
ICEBERG_ASSIGN_OR_RAISE(table_metadata->last_partition_id,
11011100
GetJsonValue<int32_t>(json, kLastPartitionId));
11021101
} else {
1103-
if (table_metadata->format_version > kFormatVersion1) {
1102+
if (table_metadata->format_version > 1) {
11041103
return JsonParseError("{} must exist in format v{}", kLastPartitionId,
11051104
table_metadata->format_version);
11061105
}
@@ -1130,7 +1129,7 @@ Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::jso
11301129
table_metadata->current_snapshot_id,
11311130
GetJsonValueOrDefault<int64_t>(json, kCurrentSnapshotId, kInvalidSnapshotId));
11321131

1133-
if (table_metadata->format_version >= kFormatVersion3) {
1132+
if (table_metadata->format_version >= 3) {
11341133
ICEBERG_ASSIGN_OR_RAISE(table_metadata->next_row_id,
11351134
GetJsonValue<int64_t>(json, kNextRowId));
11361135
} else {

‎src/iceberg/manifest/manifest_writer.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -285,17 +285,17 @@ Result<std::unique_ptr<ManifestWriter>> ManifestWriter::MakeWriter(
285285
std::optional<int64_t> writer_first_row_id = std::nullopt;
286286

287287
switch (format_version) {
288-
case kFormatVersion1: {
288+
case 1: {
289289
adapter = std::make_unique<ManifestEntryAdapterV1>(
290290
snapshot_id, std::move(partition_spec), std::move(current_schema));
291291
break;
292292
}
293-
case kFormatVersion2: {
293+
case 2: {
294294
adapter = std::make_unique<ManifestEntryAdapterV2>(
295295
snapshot_id, std::move(partition_spec), std::move(current_schema), content);
296296
break;
297297
}
298-
case kFormatVersion3: {
298+
case 3: {
299299
adapter = std::make_unique<ManifestEntryAdapterV3>(
300300
snapshot_id, first_row_id, std::move(partition_spec), std::move(current_schema),
301301
content);
@@ -359,18 +359,18 @@ Result<std::unique_ptr<ManifestListWriter>> ManifestListWriter::MakeWriter(
359359
std::unique_ptr<ManifestFileAdapter> adapter;
360360

361361
switch (format_version) {
362-
case kFormatVersion1: {
362+
case 1: {
363363
adapter = std::make_unique<ManifestFileAdapterV1>(snapshot_id, parent_snapshot_id);
364364
break;
365365
}
366-
case kFormatVersion2: {
366+
case 2: {
367367
ICEBERG_PRECHECK(sequence_number.has_value(),
368368
"Sequence number is required for format version 2");
369369
adapter = std::make_unique<ManifestFileAdapterV2>(snapshot_id, parent_snapshot_id,
370370
sequence_number.value());
371371
break;
372372
}
373-
case kFormatVersion3: {
373+
case 3: {
374374
ICEBERG_PRECHECK(sequence_number.has_value(),
375375
"Sequence number is required for format version 3");
376376
ICEBERG_PRECHECK(first_row_id.has_value(),

‎src/iceberg/table_metadata.cc‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -293,8 +293,7 @@ Result<std::shared_ptr<Snapshot>> TableMetadata::SnapshotById(int64_t snapshot_i
293293
}
294294

295295
int64_t TableMetadata::NextSequenceNumber() const {
296-
return format_version > kFormatVersion1 ? last_sequence_number + 1
297-
: kInitialSequenceNumber;
296+
return format_version > 1 ? last_sequence_number + 1 : kInitialSequenceNumber;
298297
}
299298

300299
namespace {
@@ -857,10 +856,9 @@ Result<int32_t> TableMetadataBuilder::Impl::AddPartitionSpec(const PartitionSpec
857856
// Get current schema and validate the partition spec against it
858857
ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata_.Schema());
859858
ICEBERG_RETURN_UNEXPECTED(spec.Validate(*schema, /*allow_missing_fields=*/false));
860-
ICEBERG_CHECK(metadata_.format_version > kFormatVersion1 ||
861-
PartitionSpec::HasSequentialFieldIds(spec),
862-
"Spec does not use sequential IDs that are required in v1: {}",
863-
spec.ToString());
859+
ICEBERG_CHECK(
860+
metadata_.format_version > 1 || PartitionSpec::HasSequentialFieldIds(spec),
861+
"Spec does not use sequential IDs that are required in v1: {}", spec.ToString());
864862

865863
ICEBERG_ASSIGN_OR_RAISE(
866864
std::shared_ptr<PartitionSpec> new_spec,
@@ -1062,7 +1060,7 @@ Status TableMetadataBuilder::Impl::AddSnapshot(std::shared_ptr<Snapshot> snapsho
10621060
ICEBERG_CHECK(!snapshots_by_id_.contains(snapshot->snapshot_id),
10631061
"Snapshot already exists for id: {}", snapshot->snapshot_id);
10641062
ICEBERG_CHECK(
1065-
metadata_.format_version == kFormatVersion1 ||
1063+
metadata_.format_version == 1 ||
10661064
snapshot->sequence_number > metadata_.last_sequence_number ||
10671065
!snapshot->parent_snapshot_id.has_value(),
10681066
"Cannot add snapshot with sequence number {} older than last sequence number {}",
@@ -1129,7 +1127,7 @@ Status TableMetadataBuilder::Impl::SetBranchSnapshotInternal(const Snapshot& sna
11291127
}
11301128

11311129
ICEBERG_CHECK(
1132-
metadata_.format_version == kFormatVersion1 ||
1130+
metadata_.format_version == 1 ||
11331131
snapshot.sequence_number <= metadata_.last_sequence_number,
11341132
"Last sequence number {} is less than existing snapshot sequence number {}",
11351133
metadata_.last_sequence_number, snapshot.sequence_number);

‎src/iceberg/test/delete_file_index_test.cc‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithUnrelatedPartitionDeletes) {
539539

540540
TEST_P(DeleteFileIndexTest, TestPartitionedTableWithOlderPartitionDeletes) {
541541
auto version = GetParam();
542-
if (version >= kFormatVersion3) {
542+
if (version >= 3) {
543543
GTEST_SKIP() << "DVs are not filtered using sequence numbers in V3+";
544544
}
545545

@@ -569,7 +569,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableWithOlderPartitionDeletes) {
569569

570570
TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalDeletes) {
571571
auto version = GetParam();
572-
if (version >= kFormatVersion3) {
572+
if (version >= 3) {
573573
GTEST_SKIP() << "Different behavior for position deletes in V3";
574574
}
575575

@@ -600,7 +600,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalDeletes) {
600600

601601
TEST_P(DeleteFileIndexTest, TestPartitionedTableScanWithGlobalAndPartitionDeletes) {
602602
auto version = GetParam();
603-
if (version >= kFormatVersion3) {
603+
if (version >= 3) {
604604
GTEST_SKIP() << "Different behavior for position deletes in V3";
605605
}
606606

@@ -673,7 +673,7 @@ TEST_P(DeleteFileIndexTest, TestPartitionedTableSequenceNumbers) {
673673

674674
TEST_P(DeleteFileIndexTest, TestUnpartitionedTableSequenceNumbers) {
675675
auto version = GetParam();
676-
if (version >= kFormatVersion3) {
676+
if (version >= 3) {
677677
GTEST_SKIP() << "Different behavior in V3";
678678
}
679679

@@ -842,7 +842,7 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeletesGroup) {
842842

843843
TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) {
844844
auto version = GetParam();
845-
if (version < kFormatVersion3) {
845+
if (version < 3) {
846846
GTEST_SKIP() << "DVs only supported in V3+";
847847
}
848848

@@ -898,7 +898,7 @@ TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) {
898898

899899
TEST_P(DeleteFileIndexTest, TestMultipleDVs) {
900900
auto version = GetParam();
901-
if (version < kFormatVersion3) {
901+
if (version < 3) {
902902
GTEST_SKIP() << "DVs only supported in V3+";
903903
}
904904

@@ -924,7 +924,7 @@ TEST_P(DeleteFileIndexTest, TestMultipleDVs) {
924924

925925
TEST_P(DeleteFileIndexTest, TestInvalidDVSequenceNumber) {
926926
auto version = GetParam();
927-
if (version < kFormatVersion3) {
927+
if (version < 3) {
928928
GTEST_SKIP() << "DVs only supported in V3+";
929929
}
930930

@@ -1177,6 +1177,6 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeleteDiscardMetrics) {
11771177
}
11781178

11791179
INSTANTIATE_TEST_SUITE_P(DeleteFileIndexVersions, DeleteFileIndexTest,
1180-
testing::Values(kFormatVersion2, kFormatVersion3));
1180+
testing::Values(2, 3));
11811181

11821182
} // namespace iceberg

‎src/iceberg/test/manifest_group_test.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ class ManifestGroupTest : public testing::TestWithParam<int8_t> {
237237

238238
TEST_P(ManifestGroupTest, CreateAndGetEntries) {
239239
auto version = GetParam();
240-
if (version < kFormatVersion2) {
240+
if (version < 2) {
241241
GTEST_SKIP() << "Delete files only supported in V2+";
242242
}
243243

@@ -485,7 +485,6 @@ TEST_P(ManifestGroupTest, PartitionFilter) {
485485
}
486486

487487
INSTANTIATE_TEST_SUITE_P(ManifestGroupVersions, ManifestGroupTest,
488-
testing::Values(kFormatVersion1, kFormatVersion2,
489-
kFormatVersion3));
488+
testing::Values(1, 2, 3));
490489

491490
} // namespace iceberg

‎src/iceberg/test/manifest_list_versions_test.cc‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -190,17 +190,17 @@ class TestManifestListVersions : public ::testing::Test {
190190
TEST_F(TestManifestListVersions, TestV1WriteDeleteManifest) {
191191
const std::string manifest_list_path = CreateManifestListPath();
192192

193-
ICEBERG_UNWRAP_OR_FAIL(auto writer, ManifestListWriter::MakeWriter(
194-
kFormatVersion1, kSnapshotId, kSnapshotId - 1,
195-
manifest_list_path, file_io_));
193+
ICEBERG_UNWRAP_OR_FAIL(auto writer,
194+
ManifestListWriter::MakeWriter(1, kSnapshotId, kSnapshotId - 1,
195+
manifest_list_path, file_io_));
196196
auto status = writer->Add(kDeleteManifest);
197197

198198
EXPECT_THAT(status, IsError(ErrorKind::kInvalidManifestList));
199199
EXPECT_THAT(status, HasErrorMessage("Cannot store delete manifests in a v1 table"));
200200
}
201201

202202
TEST_F(TestManifestListVersions, TestV1Write) {
203-
auto manifest = WriteAndReadManifestList(kFormatVersion1);
203+
auto manifest = WriteAndReadManifestList(1);
204204

205205
// V3 fields are not written and are defaulted
206206
EXPECT_FALSE(manifest.first_row_id.has_value());
@@ -224,7 +224,7 @@ TEST_F(TestManifestListVersions, TestV1Write) {
224224
}
225225

226226
TEST_F(TestManifestListVersions, TestV2Write) {
227-
auto manifest = WriteAndReadManifestList(kFormatVersion2);
227+
auto manifest = WriteAndReadManifestList(2);
228228

229229
// V3 fields are not written and are defaulted
230230
EXPECT_FALSE(manifest.first_row_id.has_value());
@@ -246,7 +246,7 @@ TEST_F(TestManifestListVersions, TestV2Write) {
246246
}
247247

248248
TEST_F(TestManifestListVersions, TestV3Write) {
249-
auto manifest = WriteAndReadManifestList(kFormatVersion3);
249+
auto manifest = WriteAndReadManifestList(3);
250250

251251
// All V3 fields should be read correctly
252252
EXPECT_EQ(manifest.manifest_path, kPath);
@@ -272,7 +272,7 @@ TEST_F(TestManifestListVersions, TestV3WriteFirstRowIdAssignment) {
272272

273273
constexpr int64_t kExpectedNextRowId = kSnapshotFirstRowId + kAddedRows + kExistingRows;
274274
auto manifest_list_path =
275-
WriteManifestList(kFormatVersion3, kExpectedNextRowId, {missing_first_row_id});
275+
WriteManifestList(3, kExpectedNextRowId, {missing_first_row_id});
276276

277277
auto manifest = ReadManifestList(manifest_list_path);
278278
EXPECT_EQ(manifest.manifest_path, kPath);
@@ -298,9 +298,8 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) {
298298
constexpr int64_t kExpectedNextRowId =
299299
kSnapshotFirstRowId + 2 * (kAddedRows + kExistingRows);
300300

301-
auto manifest_list_path =
302-
WriteManifestList(kFormatVersion3, kExpectedNextRowId,
303-
{missing_first_row_id, kTestManifest, missing_first_row_id});
301+
auto manifest_list_path = WriteManifestList(
302+
3, kExpectedNextRowId, {missing_first_row_id, kTestManifest, missing_first_row_id});
304303

305304
auto manifests = ReadAllManifests(manifest_list_path);
306305
EXPECT_EQ(manifests.size(), 3);
@@ -330,7 +329,7 @@ TEST_F(TestManifestListVersions, TestV3WriteMixedRowIdAssignment) {
330329

331330
TEST_F(TestManifestListVersions, TestV1ForwardCompatibility) {
332331
std::string manifest_list_path =
333-
WriteManifestList(kFormatVersion1, kSnapshotFirstRowId, {kTestManifest});
332+
WriteManifestList(1, kSnapshotFirstRowId, {kTestManifest});
334333
std::string expected_array_json = R"([
335334
["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null]
336335
])";
@@ -342,7 +341,7 @@ TEST_F(TestManifestListVersions, TestV2ForwardCompatibility) {
342341
// V2 manifest list files can be read by V1 readers, but the sequence numbers and
343342
// content will be ignored.
344343
std::string manifest_list_path =
345-
WriteManifestList(kFormatVersion2, kSnapshotFirstRowId, {kTestManifest});
344+
WriteManifestList(2, kSnapshotFirstRowId, {kTestManifest});
346345
std::string expected_array_json = R"([
347346
["s3://bucket/table/m1.avro", 1024, 1, 987134631982734, 2, 343, 1, [], 5292, 857273, 22910, null]
348347
])";
@@ -445,8 +444,7 @@ TEST_F(TestManifestListVersions, TestManifestsPartitionSummary) {
445444
};
446445

447446
// Test for all format versions
448-
for (int8_t format_version = kFormatVersion1; format_version <= kFormatVersion3;
449-
++format_version) {
447+
for (int8_t format_version = 1; format_version <= 3; ++format_version) {
450448
int64_t expected_next_row_id = kSnapshotFirstRowId +
451449
manifest.added_rows_count.value() +
452450
manifest.existing_rows_count.value();

‎src/iceberg/test/manifest_reader_stats_test.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,6 @@ TEST_P(TestManifestReaderStats, TestReadEntriesWithSelectCertainStatNotProjectSt
217217
}
218218

219219
INSTANTIATE_TEST_SUITE_P(ManifestReaderStatsVersions, TestManifestReaderStats,
220-
testing::Values(kFormatVersion1, kFormatVersion2,
221-
kFormatVersion3));
220+
testing::Values(1, 2, 3));
222221

223222
} // namespace iceberg

0 commit comments

Comments
 (0)