feat: Permit SQL based caches to drop dependent objects when swapping temp tables with final ones #629
Conversation
nicob3y
changed the title
📝 WalkthroughWalkthroughThe changes introduce a new boolean attribute Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Processor as PostgresSqlProcessor
participant DB as Database
Client->>Processor: _swap_temp_table_with_final_table(stream_name, temp_table_name, final_table_name)
alt Missing table names
Processor-->>Client: Raise Exception
else Valid table names
Processor->>DB: Rename final table -> deletion alias
Processor->>DB: Rename temporary table -> final table name
Processor->>DB: Drop deletion table (using CASCADE if drop_cascade=True)
Processor-->>Client: Operation completed successfully
end
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nicob3y
changed the title
|
@nicob3y - Thank you for this contribution! The change looks good to me except I don't think every SQL dialect supports CASCADE. Could we implement this specifically on Postgres and any other specific cache implementations(s) where needed? |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
airbyte/_processors/sql/postgres.py (1)
79-108:⚠️ Potential issueThe table swapping implementation looks good, but there's a missing import.
The implementation for swapping tables is well-structured with clear error handling and SQL generation. However, there's an undefined name
excused on lines 91 and 93 for error handling, which is causing pipeline failures.You need to import the exceptions module. Looking at the error handling pattern, you might be missing an import like:
+ from airbyte.exceptions import excor possibly:
+ from airbyte import exceptions as excDepending on where your exceptions are defined. What do you think?
Also, small note about the docstring - it mentions requiring MERGE support, but the implementation is using RENAME and DROP operations instead. Would it make sense to update the docstring to be more aligned with the actual implementation, wdyt?
🧰 Tools
🪛 Ruff (0.8.2)
91-91: Undefined name
exc(F821)
93-93: Undefined name
exc(F821)
🪛 GitHub Actions: Run Linters
[error] 91-93: Name 'exc' is not defined. (name-defined)
🧹 Nitpick comments (1)
airbyte/_processors/sql/postgres.py (1)
95-95: Unused parameterThe line
_ = stream_nameindicates thatstream_nameis unused in this method. Consider either:
- Removing it from the method parameters if it's truly not needed, or
- Adding a comment explaining why it's required as a parameter but not used in this implementation
Is the
stream_nameparameter needed for future extensions or for compatibility with other implementations? If not, maybe we could simplify the signature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/_processors/sql/postgres.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
airbyte/_processors/sql/postgres.py
91-91: Undefined name exc
(F821)
93-93: Undefined name exc
(F821)
🪛 GitHub Actions: Run Linters
airbyte/_processors/sql/postgres.py
[error] 91-93: Name 'exc' is not defined. (name-defined)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
airbyte/_processors/sql/postgres.py (2)
27-28: Nice addition of thedrop_cascadeparameter!This configuration parameter aligns perfectly with the PR objective of permitting SQL-based caches to drop dependent objects. The default value of
Falsemaintains backward compatibility while giving users the flexibility to enable CASCADE drops when needed. The docstring is clear and concise too.
103-105: Good implementation of the conditional CASCADEThe implementation correctly uses the
drop_cascadeconfiguration to conditionally add the CASCADE option when dropping tables. This is exactly what was needed according to the PR objectives.The ternary expression is concise and readable. This will work perfectly for users who need to drop dependent objects like views when refreshing their caches.
|
@aaronsteers How to use it: |
In the case we use the cache as a built-in destination we might expect that table-dependent objects would be droppped during a refresh.
Use case is also described on the Postgres destination connector (https://docs.airbyte.com/integrations/destinations/postgres#creating-dependent-objects)
Summary by CodeRabbit