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

Ability to use custom partition key and primary keys differs from sorting keys for *MergeTree #48

Merged

Conversation

antonio-antuan
Copy link
Contributor

  • _get_server_info implemented
  • declarative engines parameters

* _get_server_info implemented
@coveralls
Copy link

coveralls commented Feb 2, 2019

Coverage Status

Coverage increased (+1.0%) to 90.577% when pulling f18ad1e on aCLr:feature/declarative_engines into d753192 on xzkostyan:master.

@xzkostyan
Copy link
Owner

Я обязательно посмотрю это через недельку, как доберусь до компьютера.

@xzkostyan xzkostyan merged commit 91453b1 into xzkostyan:master Mar 1, 2019
@xzkostyan
Copy link
Owner

Great job.

One question: why we need _get_server_version_info and why it is muted in test_parse_date_types?

@antonio-antuan
Copy link
Contributor Author

_get_server_version_info appears in my code before that commit. The problem was that DESCRIBE TABLE returns different number of columns in different versions. Don't remember, what was the solution, but _get_server_version_info was needed. Anyway, there is no reason not to retrieve that info.

That method muted for test_parse_date_types because ALL requests to localhost:8123 will return specified response. As you can see, the response a\nDate\n2012-10-25\n is incorrect server version :)

P.S. If you going to make a new release... I'm going to make several new pull requests (joins, engines reflection and maybe something else) in a short time. Maybe you want to wait for it.

@xzkostyan
Copy link
Owner

It seems that the problem with DESCRIBE TABLE was fixed in this commit: f3e30ea

Take your time with making PRs. There is no need to hurry. I'm planning to make new release by the end of March.

@antonio-antuan
Copy link
Contributor Author

Yep, I've seen it.
If there is no reason to have that method implemented it can be removed, of sure. But it is the part of SQLA-engines API, so I think that it can exist in that dialect.

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