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

Add parameter chunk_size and default as 1 #315

Merged
merged 13 commits into from
Jun 7, 2022

Conversation

MIracleyin
Copy link
Contributor

No description provided.

Copy link
Member

@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.

And of course test is required for this feature. See existing tests for execute_iter.

clickhouse_driver/client.py Outdated Show resolved Hide resolved
@@ -376,11 +377,11 @@ def execute_iter(
"""

with self.disconnect_on_error(query, settings):
return self.iter_process_ordinary_query(
return chunks(self.iter_process_ordinary_query(
Copy link
Member

Choose a reason for hiding this comment

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

If chunk_size is 1 it should be no additional overhead

rv = self.iter_process_ordinary_query(...)

return chunks(rv, chunk_size) if chunk_size > 1 else rv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's much better.

@MIracleyin
Copy link
Contributor Author

And of course test is required for this feature. See existing tests for execute_iter.

I am sorry, but I have no idea for testing it. May I use docker images that contain clickhouse and use Class tests.test_blocks.IteratorTestCase? I am a flashman as a contributor.

@xzkostyan
Copy link
Member

Quite simple test for emitting execute_iter with chunk_size and asserting that size of chunk will be enough.

You can also run tests locally: https://clickhouse-driver.readthedocs.io/en/latest/development.html

@coveralls
Copy link

coveralls commented Jun 3, 2022

Coverage Status

Coverage increased (+0.0009%) to 96.583% when pulling 4bb0495 on MIracleyin:master into aeae5c4 on mymarilyn:master.

@MIracleyin
Copy link
Contributor Author

Oh, I see. I overlook development and look through How to Contribute.

@MIracleyin
Copy link
Contributor Author

Hi, i using https://raw.githubusercontent.com/mymarilyn/clickhouse-driver/master/tests/docker-compose.yml. I replace "$ORG" to yandex and "$VERSION" to 21.12.3.32. When it started, I run py.test -v. But I get a lot of Error: RuntimeError: Error during communication. b'Code: 210. DB::NetException: Connection refused (localhost:9000). (NETWORK_ERROR)\n\n'

@MIracleyin
Copy link
Contributor Author

Quite simple test for emitting execute_iter with chunk_size and asserting that size of chunk will be enough.

You can also run tests locally: https://clickhouse-driver.readthedocs.io/en/latest/development.html

And I also reference https://clickhouse-driver.readthedocs.io/en/latest/development.html and try it in docker, but I got the same Error. For test clickhouse, I user curl 127.0.0.1:9000 and get this
Port 9000 is for clickhouse-client program
You must use port 8123 for HTTP.

@xzkostyan
Copy link
Member

Okay. You can use github actions in your forked repo: https://clickhouse-driver.readthedocs.io/en/latest/development.html#github-actions-in-forked-repository

tests/test_blocks.py Outdated Show resolved Hide resolved
@xzkostyan xzkostyan merged commit 76b4ed6 into mymarilyn:master Jun 7, 2022
@xzkostyan
Copy link
Member

Good job!

@MIracleyin
Copy link
Contributor Author

Good job!

Thanks for you! I learn a lot of skills about contribution.

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