Skip to content
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

feat(clickhouse): add support for ALTER TABLE REPLACE PARTITION statement #3441

Merged

Conversation

GaliFFun
Copy link
Contributor

@GaliFFun GaliFFun commented May 9, 2024

Added support for ClickHouse ALTER TABLE REPLACE PARTITION, which is very useful in ETL processes (which my company analyzes using sqlglot). ON CLUSTER support isn't there yet, willing to do that in the near future.

Doc reference:
https://clickhouse.com/docs/en/sql-reference/statements/alter/partition#replace-partition

sqlglot/dialects/clickhouse.py Outdated Show resolved Hide resolved
sqlglot/dialects/clickhouse.py Outdated Show resolved Hide resolved
@VaggelisD
Copy link
Collaborator

VaggelisD commented May 9, 2024

Thanks for the PRs and for the responsive iterations @GaliFFun !

By looking at the Clickhouse docs, it seems that there's a long list of partition-related ALTERs with repeating specs, which will clutter expressions.py. Do we need that level of granularity? Would it make sense to have a catch-all exp.AlterPartition expression with an extra kind keyword storing the action e.g. ATTACH / FREEZE / UNFREEZE etc?

Also, for the [ON CLUSTER <cluster>], I think you can solve it across board in parser.py::_parse_alter() by calling _parse_on_property() if ON is matched right before you visit the respective alter parsers.

@GaliFFun
Copy link
Contributor Author

GaliFFun commented May 9, 2024

@VaggelisD this seems like a good idea. The reason I used the current approach is the existence of exp.DropPartition. Which of the statements do you think should be included in exp.AlterPartition? I can modify the PR to include those and ON CLUSTER support as well, but it will take some time

@VaggelisD
Copy link
Collaborator

VaggelisD commented May 9, 2024

The ON CLUSTER will not be a big problem, if you want to take a stab at it in this PR feel free and we can step in to help, otherwise we can revisit it in the future.

As for the specialized vs catch-all expressions, no matter the approach I think it still be good to limit the scope of each PR (as you've done already), so don't feel pressured to add everything in at once right away.

Let's wait until others chime in on this issue

@georgesittas
Copy link
Collaborator

We can refactor this approach in a followup PR to avoid scope creep. I like Vaggelis' idea, let's see if we can avoid having to add all these new expression types for each of these ALTER statement variants.

@georgesittas georgesittas merged commit 07badc9 into tobymao:main May 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants