Added escaping for column name when backup is created#327
Open
EilRoviSoft wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideAdds a preprocessing step to sanitize ClickHouse CREATE/ATTACH statements by backtick-escaping specific unescaped reserved keywords (currently TOP) before executing them during table restore, along with unit tests to validate the behavior and ensure no changes for already valid DDL. Sequence diagram for ClickhouseCTL.create_table DDL fixing on restoresequenceDiagram
participant ClickhouseCTL
participant _fix_create_statement
participant CHClient as ClickhouseClient
ClickhouseCTL->>ClickhouseCTL: create_table(table)
ClickhouseCTL->>_fix_create_statement: _fix_create_statement(table.create_statement)
_fix_create_statement-->>ClickhouseCTL: fixed_statement
ClickhouseCTL->>CHClient: query(fixed_statement)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
aalexfvk
marked this pull request as ready for review
May 28, 2026 07:40
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
logging.warningcall in_fix_create_statementuses{!r}-style placeholders, butloggingexpects%-style formatting; this will log the braces literally, so switch to%rplaceholders or explicitstr.formatbefore passing the message. - Since
_fix_create_statementis applied to everycreate_statement, consider constraining the regex or adding an early guard so that only statements containing the affected keyword patterns (e.g.,TOP AS) are processed, reducing the chance of unintended rewrites in other DDL forms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `logging.warning` call in `_fix_create_statement` uses `{!r}`-style placeholders, but `logging` expects `%`-style formatting; this will log the braces literally, so switch to `%r` placeholders or explicit `str.format` before passing the message.
- Since `_fix_create_statement` is applied to every `create_statement`, consider constraining the regex or adding an early guard so that only statements containing the affected keyword patterns (e.g., ` TOP AS `) are processed, reducing the chance of unintended rewrites in other DDL forms.
## Individual Comments
### Comment 1
<location path="tests/unit/test_control.py" line_range="58-67" />
<code_context>
assert _parse_version(version) == expected
+
+
+def test_fix_create_statement_top_keyword():
+ broken = (
+ "ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' "
+ "(c String) AS WITH A AS (SELECT TOP AS c FROM default.test_3) "
+ "SELECT c FROM A"
+ )
+ fixed = _fix_create_statement(broken)
+
+ assert "`TOP`" in fixed
+ assert "SELECT `TOP` AS c" in fixed
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions by checking the full fixed statement rather than only substrings.
Substring checks would still pass if `_fix_create_statement` made other unintended changes to the DDL. Define a full `expected_fixed` statement and assert `fixed == expected_fixed` so the test fully specifies the transformation and better catches regressions.
```suggestion
def test_fix_create_statement_top_keyword() -> None:
broken = (
"ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' "
"(c String) AS WITH A AS (SELECT TOP AS c FROM default.test_3) "
"SELECT c FROM A"
)
expected_fixed = (
"ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' "
"(c String) AS WITH A AS (SELECT `TOP` AS c FROM default.test_3) "
"SELECT c FROM A"
)
fixed = _fix_create_statement(broken)
assert fixed == expected_fixed
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+58
to
+67
| def test_fix_create_statement_top_keyword(): | ||
| broken = ( | ||
| "ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' " | ||
| "(c String) AS WITH A AS (SELECT TOP AS c FROM default.test_3) " | ||
| "SELECT c FROM A" | ||
| ) | ||
| fixed = _fix_create_statement(broken) | ||
|
|
||
| assert "`TOP`" in fixed | ||
| assert "SELECT `TOP` AS c" in fixed |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen assertions by checking the full fixed statement rather than only substrings.
Substring checks would still pass if _fix_create_statement made other unintended changes to the DDL. Define a full expected_fixed statement and assert fixed == expected_fixed so the test fully specifies the transformation and better catches regressions.
Suggested change
| def test_fix_create_statement_top_keyword(): | |
| broken = ( | |
| "ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' " | |
| "(c String) AS WITH A AS (SELECT TOP AS c FROM default.test_3) " | |
| "SELECT c FROM A" | |
| ) | |
| fixed = _fix_create_statement(broken) | |
| assert "`TOP`" in fixed | |
| assert "SELECT `TOP` AS c" in fixed | |
| def test_fix_create_statement_top_keyword() -> None: | |
| broken = ( | |
| "ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' " | |
| "(c String) AS WITH A AS (SELECT TOP AS c FROM default.test_3) " | |
| "SELECT c FROM A" | |
| ) | |
| expected_fixed = ( | |
| "ATTACH VIEW test_4 UUID '1c465905-164d-400b-8a39-ff382c92fb81' " | |
| "(c String) AS WITH A AS (SELECT `TOP` AS c FROM default.test_3) " | |
| "SELECT c FROM A" | |
| ) | |
| fixed = _fix_create_statement(broken) | |
| assert fixed == expected_fixed |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by Sourcery
Handle ClickHouse DDL with unescaped reserved keywords during restore to prevent failures when attaching views or tables.
Bug Fixes:
Tests: