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 missing UInt128 (UUID) field visitors #2618

Merged
merged 1 commit into from
Aug 3, 2018
Merged

Add missing UInt128 (UUID) field visitors #2618

merged 1 commit into from
Aug 3, 2018

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jul 9, 2018

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

TBH, I have just a little idea what I have done here but I tried to address those #1770, #2611

It is not possible to use UUID in WHERE condition. I tried to implement it but need help with that. Not sure if the way I have tried to do this is the right way and whether I should continue, until I resolve all errors or whether it needs another solution.

@alexey-milovidov alexey-milovidov self-requested a review July 9, 2018 23:40
@alexey-milovidov
Copy link
Member

That looks mostly correct.

Only few issues remain:

  1. I cannot find implementation of FieldVisitorToString for UInt128. That will likely require some effort.
  2. Comparison (less/greater) operators for UInt128 may work even with your implementation (that returns false when comparing with other integer type) but it will confuse index when user write something like x > 1 where x is UInt128. Better to provide correct implementation from the beginning.

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

.

@simPod
Copy link
Contributor Author

simPod commented Jul 10, 2018

  1. it's implemented in https://github.com/yandex/ClickHouse/blob/master/dbms/src/Functions/FunctionsCoding.h#L893, couldn't it be somehow leveraged?
  2. hm, I'm afraid I'm out of skill for that, first time editing C code. Maybe someone can cherrypick and continue the work here?

alexey-milovidov added a commit that referenced this pull request Aug 3, 2018
@alexey-milovidov alexey-milovidov merged commit 6c7ba03 into ClickHouse:master Aug 3, 2018
@alexey-milovidov
Copy link
Member

The implementation is still incomplete, but it is better than nothing and now #2611 is fixed.

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.

2 participants