Allows hex() to work on type of UUID #32170
Conversation
Signed-off-by: frank chen <[email protected]>
Signed-off-by: frank chen <[email protected]>
Signed-off-by: frank chen <[email protected]>
robot-clickhouse
added
the
pr-improvement
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@Mergifyio update |
✅ Branch has been successfully updated |
| @@ -11,3 +11,13 @@ with generateUUIDv4() as uuid, | |||
| identity(lower(hex(reverse(reinterpretAsString(uuid))))) as str, | |||
| reinterpretAsUUID(reverse(unhex(str))) as uuid2 | |||
| select uuid = uuid2; | |||
|
|
|||
| with generateUUIDv4() as uuid, | |||
There was a problem hiding this comment.
@FrankChen021 better to add new test, without modifying old one and fix it. Please check CI report.
There was a problem hiding this comment.
Let me check why it fails. The test passed before I opened the PR.
There was a problem hiding this comment.
I found the answer. Original hex/bin functions don't output the leading zero for integers, so if each 64bit of UUID has leading zeros, they're also skipped. This might lead to test failure because UUID is generated randomly, and in that case, I think the generated UUID contains leading zeros.
For UUID's hex output, the leading zero should also be output.
There was a problem hiding this comment.
@FrankChen021 can't we use string implementation instead of trying to reuse integer implementations ?
Also new changes in tests should be moved to new tests, that way it is easier to track where tests start to fail
There was a problem hiding this comment.
We can't use the string implementation. Because we want to get the result in big-endian order, while the string implementation still generates a string in little-endian order.
Signed-off-by: frank chen <[email protected]>
Signed-off-by: frank chen <[email protected]>
|
Following Fast test fails: 01053_window_view_proc_hop_to_now | FAIL Following Stateless tests fails: 00993_system_parts_race_condition_drop_zookeeper | FLAKY They seem have nothing to do with the changes in this PR. |
|
But we still need green CI to do merge. |
|
@alexey-milovidov I understand it. But my question is even the current master branch does not pass all the CI checks, how could I ensure the checks on my branch will be passed after I merge the master into my branch? |
|
10 days ago one test in the master branch was broken and quickly fixed. |
alexey-milovidov
added
the
can be tested
|
@alexey-milovidov Some checks failed because they didn't successfully start. What should I do now? |
|
@Mergifyio update |
✅ Branch has been successfully updated |
| select str1 = str2; | ||
|
|
||
| -- hex on UUID always generate 32 characters even there're leading zeros | ||
| select lower(hex(toUUID('00000000-80e7-46f8-0000-9d773a2fd319'))); |
There was a problem hiding this comment.
I prefer if the order of output characters will be exactly the same as in UUID:
0000000080e746f800009d773a2fd319
There was a problem hiding this comment.
Actually, this is how toUUID treats the input string. toUUID treats the input string as a little-endian order string.
|
It will be very strange if hex/unhex for UUID will use different byte order that UUID has in its text representation. |
|
@alexey-milovidov Maybe you're right. Let me take a consideration. |
Signed-off-by: frank chen <[email protected]>
Signed-off-by: Frank Chen <[email protected]>
|
@alexey-milovidov @kitaisreal Your comments have been addressed in the latest commit. Please take a look at it at your |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added
UUIDdata type support for functionshex,bin.To simplify the conversion of UUID to string.
And
lower(hex(uuid))equals toreplace(toString(uuid), '-', '')And bin/hex share the same code abstraction, the change in this patch also allows bin() to work on type of UUID.