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

Rewrite DELETE docs #55856

Merged
merged 4 commits into from
Oct 31, 2023
Merged

Rewrite DELETE docs #55856

merged 4 commits into from
Oct 31, 2023

Conversation

justindeguzman
Copy link
Contributor

@justindeguzman justindeguzman commented Oct 20, 2023

Changelog category (leave one):

  • Documentation (changelog entry is not required)

@robot-clickhouse-ci-2
Copy link
Contributor

robot-clickhouse-ci-2 commented Oct 20, 2023

This is an automated comment for commit 7738f3a with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

Successful checks
Check nameDescriptionStatus
Docs CheckBuilds and tests the documentation✅ success
Mergeable CheckChecks if all other necessary checks are successful✅ success
Push to DockerhubThe check for building and pushing the CI related docker images to docker hub✅ success
Style CheckRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success
Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending

Copy link
Contributor

@thomoco thomoco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice set of changes, thank you

I was wondering - should we perhaps be recommending wide parts in all use cases where many deletions are expected?

Copy link
Member

@evillique evillique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also compare it to the ALTER TABLE ... DELETE in ways that ALTER DELETE will actually delete the data, but it will recreate all the affected parts, which may take a lot of resources at the moment of deletion. At the same time, the DELETE query will not load ClickHouse as much because we only mark the rows as deleted, but it may take a lot of time for the data to be actually deleted.

docs/en/sql-reference/statements/delete.md Outdated Show resolved Hide resolved
docs/en/sql-reference/statements/delete.md Outdated Show resolved Hide resolved
docs/en/sql-reference/statements/delete.md Outdated Show resolved Hide resolved
docs/en/sql-reference/statements/delete.md Outdated Show resolved Hide resolved
docs/en/sql-reference/statements/delete.md Outdated Show resolved Hide resolved
@justindeguzman justindeguzman removed the request for review from DerekChia October 23, 2023 15:40
@justindeguzman
Copy link
Contributor Author

@evillique Thank you for reviewing. I believe I've addressed all of your feedback above in the most recent commit.

Please let me know if there are any more changes I should make.

@den-crane
Copy link
Contributor

den-crane commented Oct 29, 2023

Very nice set of changes, thank you

I was wondering - should we perhaps be recommending wide parts in all use cases where many deletions are expected?

May be it has sense to put _row_exists into a separate file always, even if a part is compact. Compact parts are introduced to solve the overhead of small inserts, but delete happens when inserts already done (no overhead) and this column does not exists in the data.bin (no need to update data in the data.bin).

Copy link
Member

@evillique evillique left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect, thank you!

@evillique evillique added the can be tested Allows running workflows for external contributors label Oct 29, 2023
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-documentation Documentation PRs for the specific code PR label Oct 29, 2023
@evillique evillique merged commit cd779d4 into master Oct 31, 2023
14 of 16 checks passed
@evillique evillique deleted the rewrite-delete-docs branch October 31, 2023 23:10
@nellicus
Copy link
Contributor

nellicus commented Nov 2, 2023

thank you @evillique @justindeguzman ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-documentation Documentation PRs for the specific code PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants