Add hilbertEncode and hilbertDecode functions#60156
Add hilbertEncode and hilbertDecode functions#60156yariks5s merged 58 commits intoClickHouse:masterfrom
hilbertEncode and hilbertDecode functions#60156Conversation
|
This is an automated comment for commit 7977de9 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
|
Hello! Thanks for the pull request! Could you please sign the CLA? |
Sure, just signed it. And i'm still working on it, think that will finish in the end of this week |
…to hilbert-function
…to hilbert-function
hilbertEncode and hilbertDecode functions
|
@Artemmm91 the documentation team has a project to document all functions. |
Okay, no problem) |
src/Functions/hilbertDecode.h
Outdated
| namespace DB | ||
| { | ||
|
|
||
| namespace HilbertDetails |
There was a problem hiding this comment.
any specific reason to use HilbertDetails namespace instead of anonymous namespace? (Also in hilbertEncode)
There was a problem hiding this comment.
AFAIC it is not recommended to make anonymous namespace in header files
…to hilbert-function
hilbertEncode and hilbertDecode functionshilbertEncode and hilbertDecode functions
|
@yariks5s there are some problems with linking with gtest_hilbert_curve, so i'll make header in which i'll store implementations for 2D and lookup tables and will include it to test and to hilbertEncode.cpp file |
|
@yariks5s Hi! i think i'm ready with PR. i see that there are some failed checks, but seems they are not correlated with my PR:
|
|
Also i'm currently working on index analysis of hilbert function - did this on my local branch, and will make other PR after merging this one |
|
|
||
| **Example** | ||
|
|
||
| If a single argument is provided with a tuple specifying bit shifts, the function shifts the argument left by the specified number of bits. |
There was a problem hiding this comment.
Generally, the Hilbert curve does not apply to a single argument (because it's generally used for coordinates). We can also mention here that the second argument is considered as zero (please correct me if I'm wrong)
There was a problem hiding this comment.
didn't quiet get you, but if you mean that we can state that hilbertEncode(N, 0) = hilbertEncode(N) that's not true
currently i'm adding the case with num_dimensions = 2, but i'm looking forward to also add the case of num_dimensions in [3, 8]. so for the completeness, i think it's good to also have the case num_dimemnsions = 1 (and mathematically speaking, it is a valid case)
| const auto leading_zeros_count = getLeadingZeroBits(x | y); | ||
| const auto used_bits = std::numeric_limits<UInt64>::digits - leading_zeros_count; | ||
| if (used_bits > 32) | ||
| return 0; // hilbert code will be overflowed in this case |
There was a problem hiding this comment.
Basically, it does mean that it will always return 0 as code, if we will try to use numbers which fit only in UInt64 and not in UInt32 ?
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add Hilbert Curve encode and decode functions
https://en.wikipedia.org/wiki/Hilbert_curve
Closes #55520.
Documentation entry for user-facing changes