Skip to content

Add missing UInt128 (UUID) field visitors#2618

Merged
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
simPod:uuid-fix
Aug 3, 2018
Merged

Add missing UInt128 (UUID) field visitors#2618
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
simPod:uuid-fix

Conversation

@simPod
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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