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

HTTP-session tests, assorted fixes #73

Merged
merged 14 commits into from
Feb 15, 2020
Merged

Conversation

hhell
Copy link
Contributor

@hhell hhell commented Jan 14, 2020

clickhouse-over-HTTP is not obsolete.

This is a preliminary pull request; merging various improvements and fixes to upstream.

@coveralls
Copy link

coveralls commented Jan 14, 2020

Coverage Status

Coverage increased (+1.0%) to 92.988% when pulling e0b5ba6 on hhell:some_fixes into 2f79283 on xzkostyan:master.

@xzkostyan
Copy link
Owner

Yes, you are right. Clickhouse-over-http is not obsolete.

I want to made http transport in this project obsolete (not fully supported). I don't have enough efforts to maintain both transports. But PRs are always appreciated!

If you are working with large datasets tcp transport is faster, than http. Types conversion object -> str or str -> object is main bottleneck.
If you are not working with large datasets you will not notice difference between http and tcp.

@hhell
Copy link
Contributor Author

hhell commented Jan 15, 2020

I don't have enough efforts to maintain both transports

Best-case is you could at least avoid breaking it (thus the tests),

worst-case – it will have to have the tests in a different fork, and PRs with fixes.

Types conversion object -> str or str -> object is main bottleneck.

That's mostly the problem of the TSV parsing;

another modification (intended for a next PR) is streamed JSONCompact parsing

(also, see ClickHouse/ClickHouse#7841 ).

@xzkostyan
Copy link
Owner

I'll try to review PR by the end of this week.

clickhouse_sqlalchemy/drivers/base.py Show resolved Hide resolved
clickhouse_sqlalchemy/drivers/base.py Outdated Show resolved Hide resolved
clickhouse_sqlalchemy/drivers/base.py Show resolved Hide resolved
tests/session.py Show resolved Hide resolved
tests/session.py Outdated Show resolved Hide resolved
tests/test_types.py Show resolved Hide resolved
tests/test_types.py Show resolved Hide resolved
tests/testcase.py Show resolved Hide resolved
@hhell
Copy link
Contributor Author

hhell commented Feb 3, 2020

bump.

@xzkostyan
Copy link
Owner

I need a little more time for brief review of whole changes. I'll return with feedback asap.

@hhell
Copy link
Contributor Author

hhell commented Feb 11, 2020

bump.

@xzkostyan xzkostyan merged commit 62b98dd into xzkostyan:master Feb 15, 2020
@xzkostyan xzkostyan mentioned this pull request Feb 15, 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.

3 participants