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

Async driver support #214

Merged
merged 11 commits into from
Jan 12, 2023
Merged

Conversation

randomowo
Copy link
Contributor

dialect mostly fully copied from sqlalchemy.dialects.mysql.asyncmy

@randomowo
Copy link
Contributor Author

need to set sqlalchemy>=1.4.24 (because of AdaptedConnection)

@randomowo
Copy link
Contributor Author

@xzkostyan Is there something needed from me for PR to be merged?

@coveralls
Copy link

coveralls commented Dec 6, 2022

Coverage Status

Coverage: 83.684% (+0.002%) from 83.682% when pulling 8723077 on randomowo:async-driver-support into 35bc4e6 on xzkostyan:master.

@xzkostyan
Copy link
Owner

@randomowo the first step is green check. Good job. It's completed now. I'll take a detailed look on this PR in weekend.

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.

Very good!

Why do we have test "core" types (Int, Ip, date) behaviour in async version of native dialect? Isn't it redundant?

clickhouse_sqlalchemy/drivers/native/base.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@randomowo
Copy link
Contributor Author

Async native dialect works a little differently (at least with different drivers) from sync, and I think we should test it separately, but on the other hand, we have types testing in the driver library. So if you still think it is redundant, I can remove it.

@xzkostyan
Copy link
Owner

Async native dialect works a little differently (at least with different drivers) from sync, and I think we should test it separately, but on the other hand, we have types testing in the driver library. So if you still think it is redundant, I can remove it.

You are right, we should test it separately. But async dialect inherited from the sync one: class ClickHouseDialect_asynch(ClickHouseDialect_native):. Type processing isn't overloaded in this dialect. Async native and sync native interfaces use clickhouse-driver under the hood. It means that we are testing the same "core part" in terms of data post-processing from underlying driver. In case of native interface types are bypassed to use, in case of HTTP interface data should be converted into Python types.

That's why, IMHO @with_native_and_http_sessions is enough for type testing for tests located here: tests/types. Tests for async behaiviour are very useful.

@randomowo
Copy link
Contributor Author

Async native dialect works a little differently (at least with different drivers) from sync, and I think we should test it separately, but on the other hand, we have types testing in the driver library. So if you still think it is redundant, I can remove it.

You are right, we should test it separately. But async dialect inherited from the sync one: class ClickHouseDialect_asynch(ClickHouseDialect_native):. Type processing isn't overloaded in this dialect. Async native and sync native interfaces use clickhouse-driver under the hood. It means that we are testing the same "core part" in terms of data post-processing from underlying driver. In case of native interface types are bypassed to use, in case of HTTP interface data should be converted into Python types.

That's why, IMHO @with_native_and_http_sessions is enough for type testing for tests located here: tests/types. Tests for async behaiviour are very useful.

ok, i'll delete it soon

@xzkostyan
Copy link
Owner

Looks good to me. Let's make linter happy.

@xzkostyan xzkostyan merged commit e2fe139 into xzkostyan:master Jan 12, 2023
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