Skip to content

Commit b9f4585

Browse files
committed
fix some comments
1 parent ebf24f4 commit b9f4585

File tree

7 files changed

+44
-26
lines changed

7 files changed

+44
-26
lines changed

src/iceberg/catalog/memory/in_memory_catalog.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "iceberg/table_requirements.h"
3030
#include "iceberg/table_update.h"
3131
#include "iceberg/transaction.h"
32+
#include "iceberg/util/checked_cast.h"
3233
#include "iceberg/util/macros.h"
3334

3435
namespace iceberg {
@@ -423,8 +424,7 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::CreateTable(
423424
ICEBERG_RETURN_UNEXPECTED(
424425
root_namespace_->UpdateTableMetadataLocation(identifier, metadata_file_location));
425426
return Table::Make(identifier, std::move(table_metadata),
426-
std::move(metadata_file_location), file_io_,
427-
std::static_pointer_cast<Catalog>(shared_from_this()));
427+
std::move(metadata_file_location), file_io_, shared_from_this());
428428
}
429429

430430
Result<std::shared_ptr<Table>> InMemoryCatalog::UpdateTable(
@@ -444,7 +444,8 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::UpdateTable(
444444
for (const auto& update : updates) {
445445
if (update->kind() == TableUpdate::Kind::kUpgradeFormatVersion) {
446446
format_version =
447-
dynamic_cast<const table::UpgradeFormatVersion&>(*update).format_version();
447+
internal::checked_cast<const table::UpgradeFormatVersion&>(*update)
448+
.format_version();
448449
}
449450
}
450451
builder = TableMetadataBuilder::BuildFromEmpty(format_version);
@@ -496,7 +497,8 @@ Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable(
496497
ICEBERG_ASSIGN_OR_RAISE(
497498
auto table, StagedTable::Make(identifier, std::move(table_metadata), "", file_io_,
498499
shared_from_this()));
499-
return Transaction::Make(std::move(table), Transaction::Kind::kCreate, false);
500+
return Transaction::Make(std::move(table), Transaction::Kind::kCreate,
501+
/* auto_commit */ false);
500502
}
501503

502504
Result<bool> InMemoryCatalog::TableExists(const TableIdentifier& identifier) const {

src/iceberg/table_identifier.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ namespace iceberg {
2626
std::string Namespace::ToString() const { return FormatRange(levels, ".", "", ""); }
2727

2828
std::string TableIdentifier::ToString() const {
29-
return std::format("{}.{}", ns.ToString(), name);
29+
if (!ns.levels.empty()) {
30+
return std::format("{}.{}", ns.ToString(), name);
31+
} else {
32+
return name;
33+
}
3034
}
3135

3236
} // namespace iceberg

src/iceberg/table_metadata.cc

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,18 +1099,18 @@ std::unique_ptr<TableMetadataBuilder> TableMetadataBuilder::BuildFromEmpty(
10991099
}
11001100

11011101
std::unique_ptr<TableMetadataBuilder> TableMetadataBuilder::BuildFrom(
1102-
const TableMetadata* base, bool is_create) {
1103-
if (is_create) {
1104-
ICEBERG_DCHECK(base != nullptr, "base should not be nullptr if is_create is true");
1105-
auto builder = BuildFromEmpty();
1106-
for (auto& change : base->ChangesForCreate()) {
1107-
change->ApplyTo(*builder);
1108-
}
1109-
return builder;
1110-
}
1102+
const TableMetadata* base) {
11111103
return std::unique_ptr<TableMetadataBuilder>(new TableMetadataBuilder(base)); // NOLINT
11121104
}
11131105

1106+
TableMetadataBuilder& TableMetadataBuilder::ApplyChangesForCreate(
1107+
const TableMetadata& base) {
1108+
for (auto& change : base.ChangesForCreate()) {
1109+
change->ApplyTo(*this);
1110+
}
1111+
return *this;
1112+
}
1113+
11141114
TableMetadataBuilder& TableMetadataBuilder::SetMetadataLocation(
11151115
std::string_view metadata_location) {
11161116
impl_->SetMetadataLocation(metadata_location);

src/iceberg/table_metadata.h

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -219,13 +219,15 @@ class ICEBERG_EXPORT TableMetadataBuilder : public ErrorCollector {
219219
/// \brief Create a builder from existing table metadata
220220
///
221221
/// \param base The base table metadata to build from
222-
/// \param is_create Whether the builder is for creating a new table. It will call
223-
/// `BuildFromEmpty` and set changes to make the tablemetadata but not copy the base
224-
/// metadata directly if true.
225222
/// \return A new TableMetadataBuilder instance initialized
226223
/// with base metadata
227-
static std::unique_ptr<TableMetadataBuilder> BuildFrom(const TableMetadata* base,
228-
bool is_create = false);
224+
static std::unique_ptr<TableMetadataBuilder> BuildFrom(const TableMetadata* base);
225+
226+
/// \brief Apply changes required to create this table metadata
227+
///
228+
/// \param base The table metadata to build from
229+
/// \return Reference to this builder for method chaining
230+
TableMetadataBuilder& ApplyChangesForCreate(const TableMetadata& base);
229231

230232
/// \brief Set the metadata location of the table
231233
///

src/iceberg/test/formatter_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ TEST(FormatterTest, TableIdentifierFormat) {
167167
.ns = Namespace({}),
168168
.name = "table_name",
169169
};
170-
EXPECT_EQ(".table_name", std::format("{}", empty_ns_table));
170+
EXPECT_EQ("table_name", std::format("{}", empty_ns_table));
171171

172172
TableIdentifier table{
173173
.ns = Namespace({"ns1", "ns2"}),

src/iceberg/transaction.cc

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@
3636

3737
namespace iceberg {
3838

39-
Transaction::Transaction(std::shared_ptr<Table> table, Kind kind, bool auto_commit)
39+
Transaction::Transaction(std::shared_ptr<Table> table, Kind kind, bool auto_commit,
40+
std::unique_ptr<TableMetadataBuilder> metadata_builder)
4041
: table_(std::move(table)),
42+
metadata_builder_(std::move(metadata_builder)),
4143
kind_(kind),
42-
auto_commit_(auto_commit),
43-
metadata_builder_(TableMetadataBuilder::BuildFrom(table_->metadata().get(),
44-
kind == Kind::kCreate)) {}
44+
auto_commit_(auto_commit) {}
4545

4646
Transaction::~Transaction() = default;
4747

@@ -50,8 +50,17 @@ Result<std::shared_ptr<Transaction>> Transaction::Make(std::shared_ptr<Table> ta
5050
if (!table || !table->catalog()) [[unlikely]] {
5151
return InvalidArgument("Table and catalog cannot be null");
5252
}
53+
54+
std::unique_ptr<TableMetadataBuilder> metadata_builder;
55+
if (kind == Kind::kCreate) {
56+
metadata_builder = TableMetadataBuilder::BuildFromEmpty();
57+
std::ignore = metadata_builder->ApplyChangesForCreate(*table->metadata());
58+
} else {
59+
metadata_builder = TableMetadataBuilder::BuildFrom(table->metadata().get());
60+
}
61+
5362
return std::shared_ptr<Transaction>(
54-
new Transaction(std::move(table), kind, auto_commit));
63+
new Transaction(std::move(table), kind, auto_commit, std::move(metadata_builder)));
5564
}
5665

5766
const TableMetadata* Transaction::base() const { return metadata_builder_->base(); }

src/iceberg/transaction.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ class ICEBERG_EXPORT Transaction : public std::enable_shared_from_this<Transacti
6969
Result<std::shared_ptr<UpdateSortOrder>> NewUpdateSortOrder();
7070

7171
private:
72-
Transaction(std::shared_ptr<Table> table, Kind kind, bool auto_commit);
72+
Transaction(std::shared_ptr<Table> table, Kind kind, bool auto_commit,
73+
std::unique_ptr<TableMetadataBuilder> metadata_builder);
7374

7475
Status AddUpdate(const std::shared_ptr<PendingUpdate>& update);
7576

0 commit comments

Comments
 (0)