Skip to content

Commit 76f878a

Browse files
suxiaogang223suxiaogang223
committed
fix: address global system table review feedback
1 parent ed5857a commit 76f878a

8 files changed

Lines changed: 244 additions & 118 deletions

File tree

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ Result<bool> FileSystemCatalog::DatabaseExists(const std::string& db_name) const
101101
Result<bool> FileSystemCatalog::TableExists(const Identifier& identifier) const {
102102
// Handle sys database global tables
103103
if (IsSystemDatabase(identifier.GetDatabaseName())) {
104-
return GlobalSystemTableLoader::IsSupported(identifier.GetTableName());
104+
return GlobalSystemTableLoader::IsSupported(identifier.GetTableName(), catalog_options_);
105105
}
106106
PAIMON_ASSIGN_OR_RAISE(bool is_system_table, identifier.IsSystemTable());
107107
if (is_system_table) {
@@ -240,7 +240,7 @@ Result<std::vector<std::string>> FileSystemCatalog::ListDatabases() const {
240240

241241
Result<std::vector<std::string>> FileSystemCatalog::ListTables(const std::string& db_name) const {
242242
if (IsSystemDatabase(db_name)) {
243-
return GlobalSystemTableLoader::GetSupportedTableNames();
243+
return GlobalSystemTableLoader::GetSupportedTableNames(catalog_options_);
244244
}
245245
std::string database_path = NewDatabasePath(warehouse_, db_name);
246246
std::vector<std::unique_ptr<BasicFileStatus>> file_status_list;
@@ -275,11 +275,13 @@ Result<std::shared_ptr<Schema>> FileSystemCatalog::LoadTableSchema(
275275
const Identifier& identifier) const {
276276
// Handle sys database global tables
277277
if (IsSystemDatabase(identifier.GetDatabaseName())) {
278-
if (!GlobalSystemTableLoader::IsSupported(identifier.GetTableName())) {
278+
PAIMON_ASSIGN_OR_RAISE(bool supported, GlobalSystemTableLoader::IsSupported(
279+
identifier.GetTableName(), catalog_options_));
280+
if (!supported) {
279281
return Status::NotExist(fmt::format("{} not exist", identifier.ToString()));
280282
}
281283
GlobalSystemTableContext context;
282-
context.catalog = const_cast<FileSystemCatalog*>(this);
284+
context.catalog = this;
283285
context.fs = fs_;
284286
context.warehouse = warehouse_;
285287
context.catalog_options = catalog_options_;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class Logger;
3939
class FileSystemCatalog : public Catalog {
4040
public:
4141
FileSystemCatalog(const std::shared_ptr<FileSystem>& fs, const std::string& warehouse,
42-
const std::map<std::string, std::string>& catalog_options = {});
42+
const std::map<std::string, std::string>& catalog_options);
4343

4444
Status CreateDatabase(const std::string& db_name,
4545
const std::map<std::string, std::string>& options,

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

Lines changed: 34 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "paimon/common/utils/path_util.h"
2929
#include "paimon/core/core_options.h"
3030
#include "paimon/core/schema/table_schema.h"
31+
#include "paimon/core/table/system/global_system_tables.h"
3132
#include "paimon/core/table/system/system_table_schema.h"
3233
#include "paimon/defs.h"
3334
#include "paimon/fs/file_system.h"
@@ -44,7 +45,7 @@ TEST(FileSystemCatalogTest, TestDatabaseExists) {
4445
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
4546
auto dir = UniqueTestDirectory::Create();
4647
ASSERT_TRUE(dir);
47-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
48+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
4849

4950
ASSERT_OK_AND_ASSIGN(auto exist, catalog.DatabaseExists("db1"));
5051
ASSERT_FALSE(exist);
@@ -70,7 +71,7 @@ TEST(FileSystemCatalogTest, TestInvalidCreateDatabase) {
7071
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
7172
auto dir = UniqueTestDirectory::Create();
7273
ASSERT_TRUE(dir);
73-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
74+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
7475

7576
ASSERT_NOK_WITH_MSG(
7677
catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true),
@@ -87,7 +88,7 @@ TEST(FileSystemCatalogTest, TestCreateSystemDatabaseAndTable) {
8788
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
8889
auto dir = UniqueTestDirectory::Create();
8990
ASSERT_TRUE(dir);
90-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
91+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
9192
ASSERT_NOK_WITH_MSG(catalog.CreateDatabase(Catalog::SYSTEM_DATABASE_NAME, options,
9293
/*ignore_if_exists=*/true),
9394
"Cannot create database for system database");
@@ -100,7 +101,7 @@ TEST(FileSystemCatalogTest, TestCreateSystemDatabaseAndTable) {
100101
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
101102
auto dir = UniqueTestDirectory::Create();
102103
ASSERT_TRUE(dir);
103-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
104+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
104105
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
105106
arrow::FieldVector fields = {
106107
arrow::field("f0", arrow::boolean()),
@@ -125,7 +126,7 @@ TEST(FileSystemCatalogTest, TestCreateTable) {
125126
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
126127
auto dir = UniqueTestDirectory::Create();
127128
ASSERT_TRUE(dir);
128-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
129+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
129130
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
130131

131132
arrow::FieldVector fields = {
@@ -161,7 +162,7 @@ TEST(FileSystemCatalogTest, TestOptionsSystemTableCatalog) {
161162
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
162163
auto dir = UniqueTestDirectory::Create();
163164
ASSERT_TRUE(dir);
164-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
165+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
165166
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
166167

167168
auto typed_schema = arrow::schema({arrow::field("f0", arrow::int32())});
@@ -219,7 +220,7 @@ TEST(FileSystemCatalogTest, TestAuditLogAndBinlogSystemTableCatalog) {
219220
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
220221
auto dir = UniqueTestDirectory::Create();
221222
ASSERT_TRUE(dir);
222-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
223+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
223224
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
224225

225226
auto typed_schema =
@@ -290,7 +291,7 @@ TEST(FileSystemCatalogTest, TestMetadataSystemTableCatalog) {
290291
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
291292
auto dir = UniqueTestDirectory::Create();
292293
ASSERT_TRUE(dir);
293-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
294+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
294295
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
295296

296297
auto typed_schema =
@@ -435,7 +436,7 @@ TEST(FileSystemCatalogTest, TestCreateTableWithBlob) {
435436
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
436437
auto dir = UniqueTestDirectory::Create();
437438
ASSERT_TRUE(dir);
438-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
439+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
439440
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
440441

441442
arrow::FieldVector fields = {arrow::field("f0", arrow::boolean()),
@@ -491,7 +492,7 @@ TEST(FileSystemCatalogTest, TestInvalidCreateTable) {
491492
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
492493
auto dir = UniqueTestDirectory::Create();
493494
ASSERT_TRUE(dir);
494-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
495+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
495496
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
496497
arrow::FieldVector fields = {
497498
arrow::field("f0", arrow::boolean()), arrow::field("f1", arrow::int8()),
@@ -516,7 +517,7 @@ TEST(FileSystemCatalogTest, TestCreateTableWhileDbNotExist) {
516517
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
517518
auto dir = UniqueTestDirectory::Create();
518519
ASSERT_TRUE(dir);
519-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
520+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
520521
arrow::FieldVector fields = {
521522
arrow::field("f0", arrow::boolean()),
522523
arrow::field("f1", arrow::int8()),
@@ -539,7 +540,7 @@ TEST(FileSystemCatalogTest, TestCreateTableWhileTableExist) {
539540
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
540541
auto dir = UniqueTestDirectory::Create();
541542
ASSERT_TRUE(dir);
542-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
543+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
543544
{
544545
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
545546
arrow::FieldVector fields = {
@@ -600,23 +601,29 @@ TEST(FileSystemCatalogTest, TestCreateTableWhileTableExist) {
600601
}
601602
}
602603

603-
TEST(FileSystemCatalogTest, TestInvalidList) {
604+
TEST(FileSystemCatalogTest, TestSystemList) {
604605
std::map<std::string, std::string> options;
605606
options[Options::FILE_SYSTEM] = "local";
606607
options[Options::FILE_FORMAT] = "orc";
607608
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
608609
auto dir = UniqueTestDirectory::Create();
609610
ASSERT_TRUE(dir);
610-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
611+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
611612
ASSERT_OK_AND_ASSIGN(auto sys_tables, catalog.ListTables("sys"));
612613
ASSERT_FALSE(sys_tables.empty());
613-
// Verify expected global system table names are present
614-
ASSERT_TRUE(std::find(sys_tables.begin(), sys_tables.end(), "catalog_options") !=
614+
// catalog_options is disabled by default, matching Java Paimon.
615+
ASSERT_TRUE(std::find(sys_tables.begin(), sys_tables.end(), "catalog_options") ==
615616
sys_tables.end());
616617
ASSERT_TRUE(std::find(sys_tables.begin(), sys_tables.end(), "all_table_options") !=
617618
sys_tables.end());
618619
ASSERT_TRUE(std::find(sys_tables.begin(), sys_tables.end(), "tables") != sys_tables.end());
619620
ASSERT_TRUE(std::find(sys_tables.begin(), sys_tables.end(), "partitions") != sys_tables.end());
621+
622+
options[CatalogOptionsSystemTable::kEnabledOption] = "true";
623+
FileSystemCatalog enabled_catalog(core_options.GetFileSystem(), dir->Str(), options);
624+
ASSERT_OK_AND_ASSIGN(auto enabled_sys_tables, enabled_catalog.ListTables("sys"));
625+
ASSERT_TRUE(std::find(enabled_sys_tables.begin(), enabled_sys_tables.end(),
626+
"catalog_options") != enabled_sys_tables.end());
620627
}
621628

622629
TEST(FileSystemCatalogTest, TestValidateTableSchema) {
@@ -626,7 +633,7 @@ TEST(FileSystemCatalogTest, TestValidateTableSchema) {
626633
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
627634
auto dir = UniqueTestDirectory::Create();
628635
ASSERT_TRUE(dir);
629-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
636+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
630637
ASSERT_OK(catalog.CreateDatabase("db1", options, /*ignore_if_exists=*/true));
631638
arrow::FieldVector fields = {
632639
arrow::field("f0", arrow::utf8()),
@@ -695,7 +702,7 @@ TEST(FileSystemCatalogTest, TestDropDatabase) {
695702
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
696703
auto dir = UniqueTestDirectory::Create();
697704
ASSERT_TRUE(dir);
698-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
705+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
699706

700707
// Test 1: Drop non-existent database with ignore_if_not_exists=true
701708
ASSERT_OK(catalog.DropDatabase("non_existent_db", /*ignore_if_not_exists=*/true,
@@ -751,7 +758,7 @@ TEST(FileSystemCatalogTest, TestDropTable) {
751758
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
752759
auto dir = UniqueTestDirectory::Create();
753760
ASSERT_TRUE(dir);
754-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
761+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
755762
ASSERT_OK(catalog.CreateDatabase("test_db", options, /*ignore_if_exists=*/false));
756763

757764
// Test 1: Drop non-existent table with ignore_if_not_exists=true
@@ -793,7 +800,7 @@ TEST(FileSystemCatalogTest, TestRenameTable) {
793800
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
794801
auto dir = UniqueTestDirectory::Create();
795802
ASSERT_TRUE(dir);
796-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
803+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
797804
ASSERT_OK(catalog.CreateDatabase("test_db", options, /*ignore_if_exists=*/false));
798805

799806
// Test 1: Rename non-existent table with ignore_if_not_exists=true
@@ -860,7 +867,7 @@ TEST(FileSystemCatalogTest, TestDropTableWithExternalPath) {
860867
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
861868
auto dir = UniqueTestDirectory::Create();
862869
ASSERT_TRUE(dir);
863-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
870+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
864871
ASSERT_OK(catalog.CreateDatabase("test_db", options, /*ignore_if_exists=*/false));
865872

866873
// Create external path directory
@@ -922,7 +929,7 @@ TEST(FileSystemCatalogTest, TestDropTableWithMultipleExternalPaths) {
922929
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
923930
auto dir = UniqueTestDirectory::Create();
924931
ASSERT_TRUE(dir);
925-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
932+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
926933
ASSERT_OK(catalog.CreateDatabase("test_db", options, /*ignore_if_exists=*/false));
927934

928935
// Create multiple external path directories
@@ -979,7 +986,7 @@ TEST(FileSystemCatalogTest, TestDropTableWithGlobalIndexExternalPathOnMainBranch
979986
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
980987
auto dir = UniqueTestDirectory::Create();
981988
ASSERT_TRUE(dir);
982-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
989+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
983990
ASSERT_OK(catalog.CreateDatabase("test_db", options, /*ignore_if_exists=*/false));
984991

985992
// Create external path directory for global index
@@ -1083,7 +1090,7 @@ TEST(FileSystemCatalogTest, TestDropTableWithGlobalIndexExternalPathOnBranch) {
10831090
ASSERT_TRUE(external_exists);
10841091

10851092
// Drop the table via catalog
1086-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
1093+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
10871094
Identifier identifier("test_db", "append_table_with_rt_branch");
10881095
ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(identifier));
10891096
ASSERT_TRUE(table_exists);
@@ -1111,7 +1118,7 @@ TEST(FileSystemCatalogTest, TestListSnapshots) {
11111118
std::string db_path = dir->Str() + "/test_db.db";
11121119
ASSERT_TRUE(TestUtil::CopyDirectory(test_data_path, db_path));
11131120

1114-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
1121+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
11151122

11161123
Identifier id("test_db", "append_table_with_multiple_file_format");
11171124
ASSERT_OK_AND_ASSIGN(std::vector<SnapshotInfo> snapshots, catalog.ListSnapshots(id, ""));
@@ -1145,7 +1152,7 @@ TEST(FileSystemCatalogTest, TestListSnapshotsTableNotExist) {
11451152
ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options));
11461153
auto dir = UniqueTestDirectory::Create();
11471154
ASSERT_TRUE(dir);
1148-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
1155+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
11491156

11501157
ASSERT_NOK_WITH_MSG(
11511158
catalog.ListSnapshots(Identifier("non_existent_db", "non_existent_table"), ""),
@@ -1203,7 +1210,7 @@ TEST(FileSystemCatalogTest, TestDropTableWithBranchExternalPaths) {
12031210
ASSERT_TRUE(external_exists);
12041211

12051212
// Drop the table via catalog
1206-
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str());
1213+
FileSystemCatalog catalog(core_options.GetFileSystem(), dir->Str(), options);
12071214
Identifier identifier("test_db", "append_table_with_rt_branch");
12081215
ASSERT_OK_AND_ASSIGN(bool table_exists, catalog.TableExists(identifier));
12091216
ASSERT_TRUE(table_exists);

0 commit comments

Comments
 (0)