-
-
Notifications
You must be signed in to change notification settings - Fork 329
fix(coordinator): drop materialized views and foreign tables with the correct DROP statement #1801
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 changes: 13 additions & 0 deletions
13
Plugins/ClickHouseDriverPlugin/ClickHousePluginDriver+TableOperations.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // | ||
| // ClickHousePluginDriver+TableOperations.swift | ||
| // ClickHouseDriverPlugin | ||
| // | ||
|
|
||
| import Foundation | ||
| import TableProPluginKit | ||
|
|
||
| extension ClickHousePluginDriver { | ||
| func dropObjectStatement(name: String, objectType: String, schema: String?, cascade: Bool) -> String? { | ||
| clickHouseDropObjectStatement(name: name, objectType: objectType) | ||
| } | ||
| } |
23 changes: 23 additions & 0 deletions
23
Plugins/ClickHouseDriverPlugin/ClickHouseTableOperations.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // | ||
| // ClickHouseTableOperations.swift | ||
| // ClickHouseDriverPlugin | ||
| // | ||
|
|
||
| import Foundation | ||
|
|
||
| func clickHouseTableType(forEngine engine: String?) -> String { | ||
| switch engine { | ||
| case "MaterializedView": | ||
| return "MATERIALIZED VIEW" | ||
| case "View", "LiveView", "WindowView": | ||
| return "VIEW" | ||
| default: | ||
| return "TABLE" | ||
| } | ||
| } | ||
|
|
||
| func clickHouseDropObjectStatement(name: String, objectType: String) -> String? { | ||
| guard objectType == "MATERIALIZED VIEW" else { return nil } | ||
| let escaped = name.replacingOccurrences(of: "`", with: "``") | ||
| return "DROP VIEW `\(escaped)`" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
119 changes: 119 additions & 0 deletions
119
TableProTests/Core/Database/TableOperationSQLBuilderTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| // | ||
| // TableOperationSQLBuilderTests.swift | ||
| // TableProTests | ||
| // | ||
|
|
||
| import Foundation | ||
| @testable import TablePro | ||
| import TableProPluginKit | ||
| import Testing | ||
|
|
||
| private final class StubDropDriver: PluginDatabaseDriver { | ||
| var supportsSchemas: Bool { false } | ||
| var supportsTransactions: Bool { false } | ||
| var currentSchema: String? { nil } | ||
| var serverVersion: String? { nil } | ||
|
|
||
| func connect() async throws {} | ||
| func disconnect() {} | ||
| func ping() async throws {} | ||
| func execute(query: String) async throws -> PluginQueryResult { | ||
| PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 0, executionTime: 0) | ||
| } | ||
|
|
||
| func fetchTables(schema: String?) async throws -> [PluginTableInfo] { [] } | ||
| func fetchColumns(table: String, schema: String?) async throws -> [PluginColumnInfo] { [] } | ||
| func fetchIndexes(table: String, schema: String?) async throws -> [PluginIndexInfo] { [] } | ||
| func fetchForeignKeys(table: String, schema: String?) async throws -> [PluginForeignKeyInfo] { [] } | ||
| func fetchTableDDL(table: String, schema: String?) async throws -> String { "" } | ||
| func fetchViewDefinition(view: String, schema: String?) async throws -> String { "" } | ||
| func fetchTableMetadata(table: String, schema: String?) async throws -> PluginTableMetadata { | ||
| PluginTableMetadata(tableName: table) | ||
| } | ||
|
|
||
| func fetchDatabases() async throws -> [String] { [] } | ||
| func fetchDatabaseMetadata(_ database: String) async throws -> PluginDatabaseMetadata { | ||
| PluginDatabaseMetadata(name: database) | ||
| } | ||
| } | ||
|
|
||
| @Suite("TableOperationSQLBuilder") | ||
| @MainActor | ||
| struct TableOperationSQLBuilderTests { | ||
| private func makeBuilder(tables: [TableInfo]) -> TableOperationSQLBuilder { | ||
| let connection = DatabaseConnection(name: "Test", type: .postgresql) | ||
| let adapter = PluginDriverAdapter(connection: connection, pluginDriver: StubDropDriver()) | ||
| return TableOperationSQLBuilder( | ||
| connectionId: connection.id, | ||
| databaseType: .postgresql, | ||
| tableInfoProvider: { Dictionary(uniqueKeysWithValues: tables.map { ($0.name, $0) }) }, | ||
| adapterProvider: { adapter } | ||
| ) | ||
| } | ||
|
|
||
| @Test("Materialized view drops with DROP MATERIALIZED VIEW") | ||
| func dropsMaterializedView() { | ||
| let builder = makeBuilder(tables: [ | ||
| TableInfo(name: "daily_sales", type: .materializedView, rowCount: nil, schema: "public") | ||
| ]) | ||
| let stmts = builder.generate(truncates: [], deletes: ["daily_sales"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP MATERIALIZED VIEW \"public\".\"daily_sales\""]) | ||
| } | ||
|
|
||
| @Test("View drops with DROP VIEW") | ||
| func dropsView() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "active_users", type: .view, rowCount: nil)]) | ||
| let stmts = builder.generate(truncates: [], deletes: ["active_users"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP VIEW \"active_users\""]) | ||
| } | ||
|
|
||
| @Test("Foreign table drops with DROP FOREIGN TABLE") | ||
| func dropsForeignTable() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "remote_orders", type: .foreignTable, rowCount: nil)]) | ||
| let stmts = builder.generate(truncates: [], deletes: ["remote_orders"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP FOREIGN TABLE \"remote_orders\""]) | ||
| } | ||
|
|
||
| @Test("Plain table drops with DROP TABLE") | ||
| func dropsTable() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "orders", type: .table, rowCount: nil)]) | ||
| let stmts = builder.generate(truncates: [], deletes: ["orders"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP TABLE \"orders\""]) | ||
| } | ||
|
|
||
| @Test("System table drops with DROP TABLE") | ||
| func dropsSystemTable() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "pg_stats", type: .systemTable, rowCount: nil)]) | ||
| let stmts = builder.generate(truncates: [], deletes: ["pg_stats"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP TABLE \"pg_stats\""]) | ||
| } | ||
|
|
||
| @Test("Unresolvable name falls back to DROP TABLE") | ||
| func fallsBackWhenLookupMisses() { | ||
| let builder = makeBuilder(tables: []) | ||
| let stmts = builder.generate(truncates: [], deletes: ["ghost"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP TABLE \"ghost\""]) | ||
| } | ||
|
|
||
| @Test("Cascade applies to materialized view drops") | ||
| func cascadeAppliesToMaterializedView() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "daily_sales", type: .materializedView, rowCount: nil)]) | ||
| let options = ["daily_sales": TableOperationOptions(cascade: true)] | ||
| let stmts = builder.generate(truncates: [], deletes: ["daily_sales"], options: options, includeFKHandling: false) | ||
| #expect(stmts == ["DROP MATERIALIZED VIEW \"daily_sales\" CASCADE"]) | ||
| } | ||
|
|
||
| @Test("Drop qualifies schema when TableInfo carries one") | ||
| func qualifiesSchema() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "orders", type: .table, rowCount: nil, schema: "sales")]) | ||
| let stmts = builder.generate(truncates: [], deletes: ["orders"], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["DROP TABLE \"sales\".\"orders\""]) | ||
| } | ||
|
|
||
| @Test("Truncate qualifies schema when TableInfo carries one") | ||
| func truncateQualifiesSchema() { | ||
| let builder = makeBuilder(tables: [TableInfo(name: "orders", type: .table, rowCount: nil, schema: "sales")]) | ||
| let stmts = builder.generate(truncates: ["orders"], deletes: [], options: [:], includeFKHandling: false) | ||
| #expect(stmts == ["TRUNCATE TABLE \"sales\".\"orders\""]) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the sidebar/database tree has objects with the same name in different schemas, the pending operation still carries only the bare table name, but this new lookup collapses
session.tablesbynameand keeps an arbitrary/firstTableInfo. Because the builder now uses thatTableInfo.schemato qualifyDROP/TRUNCATE, selectingsales.orderscan preview or execute againstpublic.ordersif that entry is first or the current schema cache is stale after a schema switch. Please carry the selected schema (or the fullTableInfo) through the pending operation instead of resolving destructive SQL by bare name.Useful? React with 👍 / 👎.