-
Notifications
You must be signed in to change notification settings - Fork 31
fix(catalog): refresh schema after table replacement #242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,16 +258,16 @@ ListRestNamespaceTables(const string &endpoint, const string &namespace_id, | |
| return out; | ||
| } | ||
|
|
||
| static bool | ||
| DirectoryNamespaceTableExists(const LanceDirectoryNamespaceConfig &ns, | ||
| const string &table_name) { | ||
| static string | ||
| FindDirectoryNamespaceTable(const LanceDirectoryNamespaceConfig &ns, | ||
| const string &table_name) { | ||
| auto tables = ListDirectoryNamespaceTables(ns); | ||
| for (auto &t : tables) { | ||
| if (StringUtil::CIEquals(t, table_name)) { | ||
| return true; | ||
| return t; | ||
| } | ||
| } | ||
| return false; | ||
| return string(); | ||
| } | ||
|
|
||
| class LanceDirectoryDefaultGenerator : public DefaultGenerator { | ||
|
|
@@ -287,14 +287,23 @@ class LanceDirectoryDefaultGenerator : public DefaultGenerator { | |
| "Unsafe Lance dataset name for directory namespace: " + entry_name); | ||
| } | ||
|
|
||
| // DuckDB resolves catalog identifiers case-insensitively, while directory | ||
| // namespace paths are case-sensitive. Resolve the requested spelling back | ||
| // to the physical table identifier before opening and materializing the | ||
| // catalog entry so an evicted Foo.lance is not rediscovered as foo.lance. | ||
| auto physical_table = FindDirectoryNamespaceTable(*ns, entry_name); | ||
| if (physical_table.empty()) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| vector<const char *> key_ptrs; | ||
| vector<const char *> value_ptrs; | ||
| BuildStorageOptionPointerArrays(ns->option_keys, ns->option_values, | ||
| key_ptrs, value_ptrs); | ||
|
|
||
| const char *uri_ptr = nullptr; | ||
| auto *dataset = lance_open_dataset_in_dir_namespace( | ||
| ns->root.c_str(), entry_name.c_str(), | ||
| ns->root.c_str(), physical_table.c_str(), | ||
| key_ptrs.empty() ? nullptr : key_ptrs.data(), | ||
| value_ptrs.empty() ? nullptr : value_ptrs.data(), | ||
| ns->option_keys.size(), &uri_ptr); | ||
|
|
@@ -307,7 +316,7 @@ class LanceDirectoryDefaultGenerator : public DefaultGenerator { | |
| return nullptr; | ||
| } | ||
|
|
||
| CreateTableInfo info(schema, entry_name); | ||
| CreateTableInfo info(schema, physical_table); | ||
| info.internal = true; | ||
| info.on_conflict = OnCreateConflict::IGNORE_ON_CONFLICT; | ||
| vector<string> coerced; | ||
|
|
@@ -321,12 +330,13 @@ class LanceDirectoryDefaultGenerator : public DefaultGenerator { | |
| lance_close_dataset(dataset); | ||
|
|
||
| if (dataset_uri.empty()) { | ||
| dataset_uri = JoinNamespacePath(ns->root, GetDatasetDirName(entry_name)); | ||
| dataset_uri = | ||
| JoinNamespacePath(ns->root, GetDatasetDirName(physical_table)); | ||
| } | ||
| LanceNamespaceTableConfig cfg; | ||
| cfg.kind = LanceNamespaceKind::Directory; | ||
| cfg.root = ns->root; | ||
| cfg.table_id = entry_name; | ||
| cfg.table_id = physical_table; | ||
| cfg.option_keys = ns->option_keys; | ||
| cfg.option_values = ns->option_values; | ||
| cfg.display_uri = std::move(dataset_uri); | ||
|
|
@@ -596,6 +606,12 @@ static string GetDatasetDirName(const string &table_name) { | |
| return table_name + ".lance"; | ||
| } | ||
|
|
||
| class LanceSchemaEntry; | ||
|
|
||
| static void EvictLanceTableCatalogEntry(LanceSchemaEntry &schema, | ||
| CatalogTransaction transaction, | ||
| const string &table_name); | ||
|
|
||
| static bool IsSafeDatasetTableName(const string &name) { | ||
| if (name.empty()) { | ||
| return false; | ||
|
|
@@ -824,13 +840,14 @@ class LanceSchemaEntry final : public DuckSchemaEntry { | |
| BuildStorageOptionPointerArrays(option_keys, option_values, key_ptrs, | ||
| value_ptrs); | ||
|
|
||
| const auto &physical_table = lance_entry->NamespaceConfig().table_id; | ||
| auto rc = lance_dir_namespace_drop_table( | ||
| root.c_str(), info.name.c_str(), | ||
| root.c_str(), physical_table.c_str(), | ||
| key_ptrs.empty() ? nullptr : key_ptrs.data(), | ||
| value_ptrs.empty() ? nullptr : value_ptrs.data(), option_keys.size()); | ||
| if (rc != 0) { | ||
| throw IOException("Failed to drop Lance dataset: " + root + "/" + | ||
| GetDatasetDirName(info.name) + | ||
| GetDatasetDirName(physical_table) + | ||
| LanceFormatErrorSuffix()); | ||
| } | ||
| } | ||
|
|
@@ -885,6 +902,7 @@ class LanceSchemaEntry final : public DuckSchemaEntry { | |
| string dataset_path; | ||
| vector<string> option_keys; | ||
| vector<string> option_values; | ||
| bool refresh_directory_entry = false; | ||
|
|
||
| if (rest_ns) { | ||
| unordered_map<string, Value> overrides; | ||
|
|
@@ -998,11 +1016,12 @@ class LanceSchemaEntry final : public DuckSchemaEntry { | |
| throw InternalException("Lance directory namespace root is empty"); | ||
| } | ||
|
|
||
| auto matched_table = | ||
| FindDirectoryNamespaceTable(*directory_ns, create_info.table); | ||
| auto exists = !matched_table.empty(); | ||
| auto physical_table = exists ? matched_table : create_info.table; | ||
| dataset_path = JoinNamespacePath(directory_ns->root, | ||
| GetDatasetDirName(create_info.table)); | ||
|
|
||
| auto exists = | ||
| DirectoryNamespaceTableExists(*directory_ns, create_info.table); | ||
| GetDatasetDirName(physical_table)); | ||
| if (create_info.on_conflict == OnCreateConflict::IGNORE_ON_CONFLICT && | ||
| exists) { | ||
| InvalidateTableDefaults(); | ||
|
|
@@ -1012,6 +1031,9 @@ class LanceSchemaEntry final : public DuckSchemaEntry { | |
| exists) { | ||
| throw IOException("Lance dataset already exists: " + dataset_path); | ||
| } | ||
| refresh_directory_entry = | ||
| exists && | ||
| create_info.on_conflict == OnCreateConflict::REPLACE_ON_CONFLICT; | ||
|
|
||
| option_keys = directory_ns->option_keys; | ||
| option_values = directory_ns->option_values; | ||
|
|
@@ -1058,6 +1080,13 @@ class LanceSchemaEntry final : public DuckSchemaEntry { | |
| LanceFormatErrorSuffix()); | ||
| } | ||
|
|
||
| if (refresh_directory_entry) { | ||
| auto cache_key = LanceBuildResolvedPathDatasetCacheKey( | ||
| dataset_path, option_keys, option_values); | ||
| LanceInvalidateDatasetCache(context, cache_key); | ||
|
mikewhb marked this conversation as resolved.
|
||
| EvictLanceTableCatalogEntry(*this, transaction, create_info.table); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The physical overwrite now targets Reproducer run on d64beffATTACH test/.tmp/gate_case_followup_d64beff AS ns (TYPE LANCE);
CREATE OR REPLACE TABLE ns.main."Foo" (id BIGINT);
INSERT INTO ns.main."Foo" VALUES (7);
CREATE OR REPLACE TABLE ns.main.foo (id BIGINT, label VARCHAR);
SELECT count(label) FROM ns.main.foo;
-- expected: 0
-- observed: Catalog Error: Table with name foo does not existThe run left only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction to the code block above: the path is a SQL string literal. The exact executed first statement was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 9a006d8: directory lookup now materializes the matched physical identifier consistently, and the focused schema-only, CTAS, and case-variant DROP regressions all pass. |
||
| } | ||
|
|
||
| // Best-effort persistence of DuckDB column defaults in Lance field | ||
| // metadata. Lance itself does not currently expose defaults through | ||
| // DuckDB's catalog, but we can still use the metadata during UPDATE | ||
|
|
@@ -1102,6 +1131,30 @@ class LanceSchemaEntry final : public DuckSchemaEntry { | |
| DefaultGenerator *table_default_generator = nullptr; | ||
| }; | ||
|
|
||
| static void EvictLanceTableCatalogEntry(LanceSchemaEntry &schema, | ||
| CatalogTransaction transaction, | ||
| const string &table_name) { | ||
| auto &set = schema.GetCatalogSet(CatalogType::TABLE_ENTRY); | ||
| auto existing_entry = set.GetEntry(transaction, table_name); | ||
| if (!existing_entry) { | ||
| return; | ||
| } | ||
| auto existing_type = existing_entry->type; | ||
| if (existing_type != CatalogType::TABLE_ENTRY && | ||
| existing_type != CatalogType::VIEW_ENTRY) { | ||
| throw InternalException( | ||
| "Unexpected catalog entry type for Lance table '%s': %s", table_name, | ||
| CatalogTypeToString(existing_type)); | ||
| } | ||
| auto system_transaction = | ||
| CatalogTransaction::GetSystemTransaction(schema.catalog.GetDatabase()); | ||
| if (!set.DropEntry(system_transaction, table_name, false, true)) { | ||
| throw InternalException("Could not drop catalog entry for Lance table '%s'", | ||
| table_name); | ||
| } | ||
| set.CleanupEntry(*existing_entry); | ||
| } | ||
|
|
||
| class LanceDuckCatalog final : public DuckCatalog { | ||
| public: | ||
| using DuckCatalog::PlanDelete; | ||
|
|
@@ -1580,11 +1633,12 @@ class LanceDuckCatalog final : public DuckCatalog { | |
| throw InternalException("Lance directory namespace root is empty"); | ||
| } | ||
|
|
||
| auto matched_table = | ||
| FindDirectoryNamespaceTable(*directory_ns, create_info.table); | ||
| auto exists = !matched_table.empty(); | ||
| auto physical_table = exists ? matched_table : create_info.table; | ||
| auto dataset_path = JoinNamespacePath(directory_ns->root, | ||
| GetDatasetDirName(create_info.table)); | ||
|
|
||
| auto exists = | ||
| DirectoryNamespaceTableExists(*directory_ns, create_info.table); | ||
| GetDatasetDirName(physical_table)); | ||
|
|
||
| if (create_info.on_conflict == OnCreateConflict::IGNORE_ON_CONFLICT && | ||
| exists) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # name: test/sql/namespace_replace_schema.test | ||
| # description: CREATE OR REPLACE TABLE replaces schema after INSERT (directory namespace) | ||
| # group: [sql] | ||
|
|
||
| require lance | ||
|
|
||
| statement ok | ||
| ATTACH 'test/.tmp/nsroot_replace_schema_126' AS ns (TYPE LANCE); | ||
|
|
||
| statement ok | ||
| CREATE OR REPLACE TABLE ns.main.replace_schema_t (id BIGINT); | ||
|
|
||
| statement ok | ||
| INSERT INTO ns.main.replace_schema_t VALUES (42); | ||
|
|
||
| statement ok | ||
| CREATE OR REPLACE TABLE ns.main.untouched_t (id BIGINT); | ||
|
|
||
| statement ok | ||
| INSERT INTO ns.main.untouched_t VALUES (7); | ||
|
|
||
| query I | ||
| SELECT count(*) FROM ns.main.replace_schema_t | ||
| ---- | ||
| 1 | ||
|
|
||
| query II | ||
| EXPLAIN (FORMAT JSON) SELECT * FROM ns.main.replace_schema_t; | ||
| ---- | ||
| physical_plan <REGEX>:[\s\S]*"Lance Dataset Cache Hit": "true"[\s\S]* | ||
|
|
||
| query II | ||
| EXPLAIN (FORMAT JSON) SELECT * FROM ns.main.untouched_t; | ||
| ---- | ||
| physical_plan <REGEX>:[\s\S]*"Lance Dataset Cache Hit": "false"[\s\S]* | ||
|
|
||
| query II | ||
| EXPLAIN (FORMAT JSON) SELECT * FROM ns.main.untouched_t; | ||
| ---- | ||
| physical_plan <REGEX>:[\s\S]*"Lance Dataset Cache Hit": "true"[\s\S]* | ||
|
|
||
| statement error | ||
| CREATE OR REPLACE TABLE ns.main.replace_schema_t (id BIGINT, label VARCHAR) | ||
| WITH (data_storage_version = 'bad_version'); | ||
| ---- | ||
| IO Error: Failed to open Lance writer: | ||
|
|
||
| query I | ||
| SELECT count(*) FROM ns.main.replace_schema_t | ||
| ---- | ||
| 1 | ||
|
|
||
| query TT | ||
| SELECT column_name, data_type FROM duckdb_columns() | ||
| WHERE database_name = 'ns' AND schema_name = 'main' AND table_name = 'replace_schema_t' | ||
| ORDER BY column_index | ||
| ---- | ||
| id BIGINT | ||
|
|
||
| statement ok | ||
| CREATE OR REPLACE TABLE ns.main.replace_schema_t (id BIGINT, label VARCHAR); | ||
|
|
||
| query II | ||
| EXPLAIN (FORMAT JSON) SELECT * FROM ns.main.replace_schema_t; | ||
| ---- | ||
| physical_plan <REGEX>:[\s\S]*"Lance Dataset Cache Hit": "false"[\s\S]* | ||
|
|
||
| query I | ||
| SELECT count(*) FROM ns.main.replace_schema_t | ||
| ---- | ||
| 0 | ||
|
|
||
| query I | ||
| SELECT count(label) FROM ns.main.replace_schema_t | ||
| ---- | ||
| 0 | ||
|
|
||
| query II | ||
| EXPLAIN (FORMAT JSON) SELECT * FROM ns.main.replace_schema_t; | ||
| ---- | ||
| physical_plan <REGEX>:[\s\S]*"Lance Dataset Cache Hit": "true"[\s\S]* | ||
|
|
||
| query II | ||
| EXPLAIN (FORMAT JSON) SELECT * FROM ns.main.untouched_t; | ||
| ---- | ||
| physical_plan <REGEX>:[\s\S]*"Lance Dataset Cache Hit": "true"[\s\S]* | ||
|
|
||
| query TT | ||
| SELECT column_name, data_type FROM duckdb_columns() | ||
| WHERE database_name = 'ns' AND schema_name = 'main' AND table_name = 'replace_schema_t' | ||
| ORDER BY column_index | ||
| ---- | ||
| id BIGINT | ||
| label VARCHAR | ||
|
|
||
| statement ok | ||
| DETACH ns; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # name: test/sql/namespace_replace_schema_case.test | ||
| # description: Case-variant replacements preserve the discovered physical table identifier | ||
| # group: [sql] | ||
|
|
||
| require lance | ||
|
|
||
| statement ok | ||
| ATTACH '__TEST_DIR__/nsroot_replace_schema_case' AS ns (TYPE LANCE); | ||
|
|
||
| statement ok | ||
| CREATE OR REPLACE TABLE ns.main."Foo" (id BIGINT); | ||
|
|
||
| statement ok | ||
| INSERT INTO ns.main."Foo" VALUES (7); | ||
|
|
||
| statement ok | ||
| CREATE OR REPLACE TABLE ns.main.foo (id BIGINT, label VARCHAR); | ||
|
|
||
| query I | ||
| SELECT count(label) FROM ns.main.foo; | ||
| ---- | ||
| 0 | ||
|
|
||
| query I | ||
| SELECT count(*) FROM '__TEST_DIR__/nsroot_replace_schema_case/Foo.lance' | ||
| ---- | ||
| 0 | ||
|
|
||
| statement error | ||
| SELECT count(*) FROM '__TEST_DIR__/nsroot_replace_schema_case/foo.lance'; | ||
| ---- | ||
| IO Error: Failed to open Lance dataset: | ||
|
|
||
| statement ok | ||
| CREATE OR REPLACE TABLE ns.main.foo AS SELECT 11::BIGINT AS id, 'ctas'::VARCHAR AS label; | ||
|
|
||
| query IT | ||
| SELECT id, label FROM ns.main.foo; | ||
| ---- | ||
| 11 ctas | ||
|
|
||
| query I | ||
| SELECT count(*) FROM '__TEST_DIR__/nsroot_replace_schema_case/Foo.lance' | ||
| ---- | ||
| 1 | ||
|
|
||
| statement error | ||
| SELECT count(*) FROM '__TEST_DIR__/nsroot_replace_schema_case/foo.lance'; | ||
| ---- | ||
| IO Error: Failed to open Lance dataset: | ||
|
|
||
| statement ok | ||
| DETACH ns; |
Uh oh!
There was an error while loading. Please reload this page.