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

LowCardinality and Nullable #14

Merged
merged 5 commits into from
Aug 31, 2020

Conversation

waralex
Copy link
Collaborator

@waralex waralex commented Aug 30, 2020

Implementation of LowCardinality and Nullable. When implementing it, I focused on the Python client. LowCardinality in C++ looks much more complex with a large number of options, but I haven't understood it in much detail yet.
While debugging LowCardinality, I made the table in ClickHouse non-consistent several times and a couple of times ClickHouse fell into the core. It seems that I have fixed everything, but I hope that you will also look at it carefully, since there is a danger of such unpleasant events

Copy link
Member

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

Interesting -- I had previously assumed that LowCardinality(T) is just a storage modifier and doesn't affect how things are sent over the network. I'm actually very glad to learn that I was wrong because the way they do it is a lot more efficient and pretty elegant.

I actually have quite a lot of LowCardinality(String) columns in tables for my projects and did a few queries on those and all of them looked good.

Code LGTM, some minor things annotated below.

src/columns/LowCardinality.jl Outdated Show resolved Hide resolved
src/columns/LowCardinality.jl Outdated Show resolved Hide resolved
src/columns/Nullable.jl Show resolved Hide resolved
Copy link
Member

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM now!

@athre0z athre0z merged commit 8b62e5b into JuliaDatabases:master Aug 31, 2020
@athre0z
Copy link
Member

athre0z commented Aug 31, 2020

Given that you now contributed like half the source code of this library, if this was still my private repo, I feel like it would only be appropriate to ask you whether you'd like to be become a proper maintainer / collaborator of this repo. However, I don't have permissions to do so and also don't really know if this is how things are done in the JuliaDatabases organization.

@quinnj it would be great if you could help us out here and provide some info on the management of this organization!

@waralex By the way, if you don't want to and rather continue just being an unassociated contributor, that is totally fine as well!

@waralex
Copy link
Collaborator Author

waralex commented Aug 31, 2020

@athre0z Of course, I would be happy to become the so-maintainer / collaborator of this repo. However this does not relieve you of the obligation to review my changes :) Just as I am ready to review any changes made by other people

@athre0z
Copy link
Member

athre0z commented Aug 31, 2020

However this does not relieve you of the obligation to review my changes :)

Heh, yes, obviously. Any non-trivial changes (trivial = typos, reformatting, ...) would still get reviewed.

@quinnj
Copy link
Member

quinnj commented Aug 31, 2020

@athre0z, I've given you admin privileges (sorry that didn't happen w/ the migration to JuliaDatabases!)
@waralex I've given you maintainer rights.

Love to see all the activity going on here! I opened #12 and assigned myself and I'll try to implement that this week.

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