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

simple delete implemented #81

Closed
wants to merge 2 commits into from
Closed

simple delete implemented #81

wants to merge 2 commits into from

Conversation

sdrenn
Copy link

@sdrenn sdrenn commented Feb 11, 2020

Like that:

t1.delete().where(t1.c.x == 25)

translates to

'ALTER TABLE t1 DELETE WHERE x = 25'

@coveralls
Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage increased (+0.07%) to 92.103% when pulling b41cff0 on sdrenn:insert into 2f79283 on xzkostyan:master.

Copy link
Owner

@xzkostyan xzkostyan left a comment

Choose a reason for hiding this comment

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

Please rebase to latest master.

Some really old servers can miss DELETE support. It was introduced in 1.1.54388.

It'll be really cool if we can check whether server supports it or not in following manner. server_version is filled on connection establishing.

self.supports_delete = self.server_version_info >= (1, 1, 54388)

If we're dealing with old server CompileError should be raised in visit_delete.


text += table_text + " DELETE"

if delete_stmt._whereclause is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that WHERE clause is required for delete:

:) alter table test delete;

Syntax error: failed at position 24 (end of query):

alter table test delete;

Expected WHERE

In case of no where exception should be raised: raise exc.CompileError('WHERE clause is required'). It's also be good to have test on this.

from tests.testcase import BaseTestCase


class DeleteTestCase(BaseTestCase):
Copy link
Owner

Choose a reason for hiding this comment

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

It's necessary to have also delete test for ORM case.

from sqlalchemy import Column

from clickhouse_sqlalchemy import types, Table
# from tests.session import session
Copy link
Owner

Choose a reason for hiding this comment

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

It seems to be redundant.

@xzkostyan xzkostyan linked an issue Mar 4, 2020 that may be closed by this pull request
@xzkostyan
Copy link
Owner

Implemented. Thanks for your efforts!

@xzkostyan xzkostyan closed this Mar 19, 2020
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.

Do you plan to implement DELETE and UPDATE statements?
3 participants