-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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 hilbertEncode
and hilbertDecode
functions
#60156
Conversation
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, it will be good to have some stateless tests that check if we encode and decode values right (examples of tests can be found in /0_stateless/.. as .sql
files).
src/Functions/hilbertDecode.h
Outdated
namespace DB | ||
{ | ||
|
||
namespace HilbertDetails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any specific reason to use HilbertDetails
namespace instead of anonymous namespace? (Also in hilbertEncode
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
230fbf7
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