Skip to content

Commit eed6225

Browse files
lucasfanglucasfang
committed
fix
1 parent e75f91a commit eed6225

4 files changed

Lines changed: 68 additions & 89 deletions

File tree

‎include/paimon/catalog/table.h‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,16 +39,6 @@ class TableRead;
3939
/// A table provides basic abstraction for table type.
4040
class PAIMON_EXPORT Table {
4141
public:
42-
/// Creates a table by loading the latest schema from the specified table path.
43-
///
44-
/// @param file_system File system used to access the table directory and schema files.
45-
/// @param table_path Root path of the table in the file system.
46-
/// @param identifier Logical table identifier selecting the schema branch and table name.
47-
/// @return A table initialized with the latest available schema.
48-
static Result<std::shared_ptr<Table>> Create(const std::shared_ptr<FileSystem>& file_system,
49-
const std::string& table_path,
50-
const Identifier& identifier);
51-
5242
/// @param file_system File system the table's data is accessed through.
5343
/// @param root_path Root path of the table in the file system.
5444
/// @param uuid Metastore UUID, or empty if unavailable.

‎src/paimon/core/catalog/file_system_catalog.cpp‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -345,9 +345,22 @@ Result<std::shared_ptr<Table>> FileSystemCatalog::GetTable(const Identifier& ide
345345
PAIMON_ASSIGN_OR_RAISE(std::string table_path, GetTableLocation(data_identifier));
346346
return std::make_shared<Table>(fs_, table_path, schema, identifier);
347347
}
348+
// The schema is read on the table's own branch: `root/branch/branch-<name>` below the
349+
// table path, which `main` resolves to itself. `GetBranchNameOrDefault` folds any
350+
// spelling of `main`, so the branch read here agrees with `CurrentBranch()`.
351+
PAIMON_ASSIGN_OR_RAISE(std::optional<std::shared_ptr<TableSchema>> latest_schema,
352+
TableSchemaExists(identifier));
353+
if (!latest_schema) {
354+
return Status::NotExist(fmt::format("{} not exist", identifier.ToString()));
355+
}
356+
// Only a managed table is a `Table`; describing another type as one would hand back
357+
// snapshots and manifests it never had, so the failure belongs to the call that opens the
358+
// table rather than at the first read.
359+
PAIMON_RETURN_NOT_OK(CatalogUtils::CheckManagedTableType(identifier, *latest_schema,
360+
"FileSystemCatalog::GetTable"));
348361
PAIMON_ASSIGN_OR_RAISE(std::string table_path, GetTableLocation(identifier));
349-
// `Table::Create` reads the same schema file and rejects a format table there.
350-
return Table::Create(fs_, table_path, identifier);
362+
return std::make_shared<Table>(fs_, table_path, checked_pointer_cast<Schema>(*latest_schema),
363+
identifier);
351364
}
352365

353366
Status FileSystemCatalog::DropDatabase(const std::string& name, bool ignore_if_not_exists,

‎src/paimon/core/table/table.cpp‎

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
#include "paimon/common/types/data_field.h"
3838
#include "paimon/common/utils/arrow/status_utils.h"
3939
#include "paimon/common/utils/binary_row_partition_computer.h"
40-
#include "paimon/common/utils/checked_cast.h"
4140
#include "paimon/common/utils/fields_comparator.h"
4241
#include "paimon/common/utils/options_utils.h"
4342
#include "paimon/common/utils/string_utils.h"
@@ -215,37 +214,6 @@ Table::Table(const std::shared_ptr<FileSystem>& file_system, const std::string&
215214
identifier_(identifier),
216215
uuid_(uuid) {}
217216

218-
Result<std::shared_ptr<Table>> Table::Create(const std::shared_ptr<FileSystem>& file_system,
219-
const std::string& table_path,
220-
const Identifier& identifier) {
221-
PAIMON_ASSIGN_OR_RAISE(bool exist, file_system->Exists(table_path));
222-
if (!exist) {
223-
return Status::NotExist(fmt::format("{} not exist", identifier.ToString()));
224-
}
225-
226-
// The schema is read on the table's own branch: `root/branch/branch-<name>` below
227-
// the table path, which `main` resolves to itself. `GetBranchNameOrDefault` folds
228-
// any spelling of `main`, so the branch read here agrees with `CurrentBranch()`.
229-
PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidTableName(identifier));
230-
PAIMON_ASSIGN_OR_RAISE(std::string branch, identifier.GetBranchNameOrDefault());
231-
SchemaManager schema_manager(file_system, table_path, branch);
232-
PAIMON_ASSIGN_OR_RAISE(std::optional<std::shared_ptr<TableSchema>> latest_schema,
233-
schema_manager.Latest());
234-
if (!latest_schema) {
235-
return Status::NotExist(
236-
fmt::format("load table schema for {} failed", identifier.ToString()));
237-
}
238-
239-
// Only a managed table is a `Table`; describing another type as one would hand back snapshots
240-
// and manifests it never had. Checked here because this is the one place every `Table` built
241-
// from a table directory passes through, catalogs included.
242-
PAIMON_RETURN_NOT_OK(
243-
CatalogUtils::CheckManagedTableType(identifier, *latest_schema, "Table::Create"));
244-
245-
auto schema = checked_pointer_cast<Schema>(*latest_schema);
246-
return std::make_shared<Table>(file_system, table_path, schema, identifier, /*uuid=*/"");
247-
}
248-
249217
Result<std::unique_ptr<TableScan>> Table::NewScan(std::unique_ptr<ScanContext> context) const {
250218
return TableScan::Create(std::move(context));
251219
}

‎src/paimon/core/table/table_test.cpp‎

Lines changed: 53 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
#include "arrow/api.h"
2828
#include "gtest/gtest.h"
2929
#include "paimon/commit_context.h"
30+
#include "paimon/common/utils/checked_cast.h"
31+
#include "paimon/core/catalog/file_system_catalog.h"
3032
#include "paimon/core/schema/schema_manager.h"
3133
#include "paimon/core/schema/table_schema.h"
3234
#include "paimon/file_store_commit.h"
@@ -44,27 +46,34 @@ namespace paimon::test {
4446

4547
namespace {
4648

47-
// Creates a managed table directory and opens it as a `Table` under `identifier`. The
48-
// schema goes to `branch`'s directory, which a branch identifier reads it back from.
49+
// Creates a managed table directory and a `Table` over it, holding the schema written to
50+
// `branch`'s directory under `identifier`.
4951
Result<std::shared_ptr<Table>> CreateManagedTable(const std::shared_ptr<FileSystem>& file_system,
5052
const std::string& path,
5153
const Identifier& identifier,
5254
const std::string& branch = "main") {
5355
SchemaManager schema_manager(file_system, path, branch);
5456
auto schema = arrow::schema({arrow::field("id", arrow::int32(), /*nullable=*/false)});
55-
PAIMON_ASSIGN_OR_RAISE([[maybe_unused]] std::unique_ptr<TableSchema> created,
57+
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<TableSchema> created,
5658
schema_manager.CreateTable(schema, /*partition_keys=*/{},
5759
/*primary_keys=*/{}, /*options=*/{}));
58-
return Table::Create(file_system, path, identifier);
60+
return std::make_shared<Table>(
61+
file_system, path,
62+
checked_pointer_cast<Schema>(std::shared_ptr<TableSchema>(std::move(created))), identifier);
5963
}
6064

6165
} // namespace
6266

63-
TEST(TableTest, TestCreateWithUnknownDatabase) {
67+
TEST(TableTest, TestGetTableWithUnknownDatabase) {
6468
auto dir = UniqueTestDirectory::Create();
6569
ASSERT_TRUE(dir);
70+
auto fs = dir->GetFileSystem();
71+
// The database the identifier names lands in its own directory below the warehouse, so the
72+
// table the catalog reads back is the one that identifier selects.
73+
const std::string table_dir = dir->Str() + "/unknown.db/tbl1";
74+
ASSERT_OK(fs->Mkdirs(table_dir));
6675

67-
SchemaManager schema_manager(dir->GetFileSystem(), dir->Str());
76+
SchemaManager schema_manager(fs, table_dir);
6877
auto schema =
6978
arrow::schema({arrow::field("id", arrow::int32(), /*nullable=*/false),
7079
arrow::field("name", arrow::utf8()), arrow::field("value", arrow::int64())});
@@ -78,9 +87,9 @@ TEST(TableTest, TestCreateWithUnknownDatabase) {
7887
ASSERT_OK_AND_ASSIGN([[maybe_unused]] std::unique_ptr<TableSchema> created_schema,
7988
schema_manager.CreateTable(schema, partition_keys, primary_keys, options));
8089

90+
FileSystemCatalog catalog(fs, dir->Str(), /*catalog_options=*/{});
8191
ASSERT_OK_AND_ASSIGN(std::shared_ptr<Table> table,
82-
Table::Create(dir->GetFileSystem(), dir->Str(),
83-
Identifier(Identifier::kUnknownDatabase, "tbl1")));
92+
catalog.GetTable(Identifier(Identifier::kUnknownDatabase, "tbl1")));
8493

8594
EXPECT_EQ(table->Name(), "tbl1");
8695
EXPECT_EQ(table->FullName(), "tbl1");
@@ -95,34 +104,35 @@ TEST(TableTest, TestCreateWithUnknownDatabase) {
95104
EXPECT_EQ(data_schema->PrimaryKeys(), primary_keys);
96105
}
97106

98-
TEST(TableTest, TestCreateWithBranch) {
107+
TEST(TableTest, TestGetTableWithBranch) {
99108
auto dir = UniqueTestDirectory::Create();
100109
ASSERT_TRUE(dir);
101110
auto fs = dir->GetFileSystem();
102-
SchemaManager schema_manager(fs, dir->Str());
111+
const std::string table_dir = dir->Str() + "/db1.db/tbl1";
112+
ASSERT_OK(fs->Mkdirs(table_dir));
113+
SchemaManager schema_manager(fs, table_dir);
103114
ASSERT_OK_AND_ASSIGN([[maybe_unused]] std::unique_ptr<TableSchema> main_schema,
104115
schema_manager.CreateTable(
105116
arrow::schema({arrow::field("main_c0", arrow::int32())}), {}, {}, {}));
106-
SchemaManager branch_schema_manager(fs, dir->Str(), "b1");
117+
SchemaManager branch_schema_manager(fs, table_dir, "b1");
107118
ASSERT_OK_AND_ASSIGN([[maybe_unused]] std::unique_ptr<TableSchema> branch_schema,
108119
branch_schema_manager.CreateTable(
109120
arrow::schema({arrow::field("b1_c0", arrow::int32())}), {}, {}, {}));
110121

122+
FileSystemCatalog catalog(fs, dir->Str(), /*catalog_options=*/{});
111123
ASSERT_OK_AND_ASSIGN(std::shared_ptr<Table> table,
112-
Table::Create(fs, dir->Str(), Identifier("db1", "tbl1$branch_b1")));
124+
catalog.GetTable(Identifier("db1", "tbl1$branch_b1")));
113125
ASSERT_EQ("tbl1$branch_b1", table->Name());
114126
ASSERT_EQ((std::vector<std::string>{"b1_c0"}), table->LatestSchema()->FieldNames());
115127
for (const auto& main_identifier :
116128
{Identifier("db1", "tbl1$branch_main"), Identifier("db1", "tbl1$branch_MAIN")}) {
117-
ASSERT_OK_AND_ASSIGN(table, Table::Create(fs, dir->Str(), main_identifier));
129+
ASSERT_OK_AND_ASSIGN(table, catalog.GetTable(main_identifier));
118130
ASSERT_EQ((std::vector<std::string>{"main_c0"}), table->LatestSchema()->FieldNames());
119131
}
120132

121-
Status missing_branch =
122-
Table::Create(fs, dir->Str(), Identifier("db1", "tbl1$branch_missing")).status();
133+
Status missing_branch = catalog.GetTable(Identifier("db1", "tbl1$branch_missing")).status();
123134
ASSERT_TRUE(missing_branch.IsNotExist()) << missing_branch.ToString();
124-
ASSERT_NOK_WITH_MSG(Table::Create(fs, dir->Str(), Identifier("db1", "tbl1$branch_../outside")),
125-
"branch");
135+
ASSERT_NOK_WITH_MSG(catalog.GetTable(Identifier("db1", "tbl1$branch_../outside")), "branch");
126136
}
127137

128138
TEST(TableTest, TestCreateHoldsTheFileSystem) {
@@ -143,27 +153,21 @@ TEST(TableTest, TestMainBranchAliasResolvesToTheMainBranch) {
143153
ASSERT_TRUE(dir);
144154
auto fs = dir->GetFileSystem();
145155
SchemaManager schema_manager(fs, dir->Str());
146-
ASSERT_OK_AND_ASSIGN([[maybe_unused]] std::unique_ptr<TableSchema> schema,
156+
ASSERT_OK_AND_ASSIGN(std::unique_ptr<TableSchema> created,
147157
schema_manager.CreateTable(
148158
arrow::schema({arrow::field("c0", arrow::int32())}), {}, {}, {}));
159+
auto schema = checked_pointer_cast<Schema>(std::shared_ptr<TableSchema>(std::move(created)));
149160

150161
for (const auto& main_identifier :
151162
{Identifier("db", "tbl"), Identifier("db", "tbl$branch_main"),
152163
Identifier("db", "tbl$branch_MAIN")}) {
153164
SCOPED_TRACE(main_identifier.ToString());
154-
ASSERT_OK_AND_ASSIGN(std::shared_ptr<Table> table,
155-
Table::Create(fs, dir->Str(), main_identifier));
156-
ASSERT_OK_AND_ASSIGN(std::string branch, table->CurrentBranch());
165+
// A catalog that loaded the schema itself calls the constructor directly, which a
166+
// rest catalog is shaped like. The alias resolves to the main branch as well.
167+
Table table(fs, dir->Str(), schema, main_identifier);
168+
ASSERT_OK_AND_ASSIGN(std::string branch, table.CurrentBranch());
157169
EXPECT_EQ("main", branch);
158170
}
159-
160-
// A catalog that loaded the schema itself calls the constructor directly, which a
161-
// rest catalog is shaped like. The alias resolves to the main branch as well.
162-
ASSERT_OK_AND_ASSIGN(std::shared_ptr<Table> plain,
163-
Table::Create(fs, dir->Str(), Identifier("db", "tbl")));
164-
Table alias_table(fs, dir->Str(), plain->LatestSchema(), Identifier("db", "tbl$branch_MAIN"));
165-
ASSERT_OK_AND_ASSIGN(std::string alias_branch, alias_table.CurrentBranch());
166-
EXPECT_EQ("main", alias_branch);
167171
}
168172

169173
TEST(TableTest, TestCurrentBranch) {
@@ -207,40 +211,44 @@ TEST(TableTest, TestCatalogOpenedTableServesThePerTableFileSystem) {
207211
EXPECT_EQ(opened->GetRootPath(), "");
208212
}
209213

210-
TEST(TableTest, TestCreateRejectsATableTypeThatIsNotAManagedTable) {
214+
TEST(TableTest, TestGetTableRejectsATableTypeThatIsNotAManagedTable) {
211215
// A `Table` describes a managed paimon table. Handing back another type as one would promise
212216
// snapshots and manifests it never had, so the failure belongs at the call that opens the
213-
// table rather than at the first read. This is the one place every `Table` built from a table
214-
// directory passes through, catalogs included.
217+
// table rather than at the first read.
215218
auto dir = UniqueTestDirectory::Create();
216219
ASSERT_TRUE(dir);
220+
auto fs = dir->GetFileSystem();
217221
auto schema = arrow::schema({arrow::field("id", arrow::int32(), /*nullable=*/false),
218222
arrow::field("dt", arrow::utf8())});
219223

220-
SchemaManager format_schema_manager(dir->GetFileSystem(), dir->Str() + "/format");
224+
FileSystemCatalog catalog(fs, dir->Str(), /*catalog_options=*/{});
225+
const std::string format_dir = dir->Str() + "/db.db/format";
226+
ASSERT_OK(fs->Mkdirs(format_dir));
227+
SchemaManager format_schema_manager(fs, format_dir);
221228
ASSERT_OK_AND_ASSIGN(
222229
[[maybe_unused]] std::unique_ptr<TableSchema> format_schema,
223230
format_schema_manager.CreateTable(schema, /*partition_keys=*/{"dt"},
224231
/*primary_keys=*/{},
225232
{{"type", "format-table"}, {"file.format", "parquet"}}));
226-
ASSERT_NOK_WITH_MSG(
227-
Table::Create(dir->GetFileSystem(), dir->Str() + "/format", Identifier("db", "fmt")),
228-
"Cannot open format table");
233+
ASSERT_NOK_WITH_MSG(catalog.GetTable(Identifier("db", "format")), "Cannot open format table");
229234

230235
// A managed table with no `type` option at all is what a `Table` is for, and still opens.
231-
SchemaManager managed_schema_manager(dir->GetFileSystem(), dir->Str() + "/managed");
236+
const std::string managed_dir = dir->Str() + "/db.db/managed";
237+
ASSERT_OK(fs->Mkdirs(managed_dir));
238+
SchemaManager managed_schema_manager(fs, managed_dir);
232239
ASSERT_OK_AND_ASSIGN([[maybe_unused]] std::unique_ptr<TableSchema> managed_schema,
233240
managed_schema_manager.CreateTable(schema, /*partition_keys=*/{"dt"},
234241
/*primary_keys=*/{}, {}));
235-
ASSERT_OK(
236-
Table::Create(dir->GetFileSystem(), dir->Str() + "/managed", Identifier("db", "managed")));
242+
ASSERT_OK(catalog.GetTable(Identifier("db", "managed")));
237243
}
238244

239-
TEST(TableTest, TestCreateFailedWithNonExistSchema) {
245+
TEST(TableTest, TestGetTableFailedWithNonExistSchema) {
240246
auto dir = UniqueTestDirectory::Create();
241247
ASSERT_TRUE(dir);
242248
auto fs = dir->GetFileSystem();
243-
SchemaManager schema_manager(fs, dir->Str());
249+
const std::string table_dir = dir->Str() + "/unknown.db/tbl1";
250+
ASSERT_OK(fs->Mkdirs(table_dir));
251+
SchemaManager schema_manager(fs, table_dir);
244252
auto schema =
245253
arrow::schema({arrow::field("id", arrow::int32(), /*nullable=*/false),
246254
arrow::field("name", arrow::utf8()), arrow::field("value", arrow::int64())});
@@ -257,10 +265,10 @@ TEST(TableTest, TestCreateFailedWithNonExistSchema) {
257265
// remove schema
258266
std::string schema_path = schema_manager.ToSchemaPath(0);
259267
ASSERT_OK(fs->Delete(schema_path, /*recursive=*/false));
260-
// check create table failed
261-
ASSERT_NOK_WITH_MSG(
262-
Table::Create(fs, dir->Str(), Identifier(Identifier::kUnknownDatabase, "tbl1")),
263-
"load table schema for Identifier{database='unknown', object='tbl1'} failed");
268+
// check get table failed
269+
FileSystemCatalog catalog(fs, dir->Str(), /*catalog_options=*/{});
270+
ASSERT_NOK_WITH_MSG(catalog.GetTable(Identifier(Identifier::kUnknownDatabase, "tbl1")),
271+
"not exist");
264272
}
265273

266274
TEST(TableTest, TestNewScanCreatesATableScan) {

0 commit comments

Comments
 (0)