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 remaining h3 miscellaneous functions #34568

Merged
merged 21 commits into from
Mar 17, 2022

Conversation

bharatnc
Copy link
Contributor

@bharatnc bharatnc commented Feb 13, 2022

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add h3PointDistM, h3PointDistKm, h3PointDistRads, h3GetRes0Indexes, h3GetPentagonIndexes functions.

This PR adds the remaining H3 miscellaneous functions as per: https://h3geo.org/docs/api/misc which are missing.

cc: @kitaisreal

Related to: #17708

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Feb 13, 2022
@kitaisreal kitaisreal self-assigned this Feb 14, 2022
Copy link
Collaborator

@kitaisreal kitaisreal left a comment

Choose a reason for hiding this comment

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

Need to generalize h3PointDistRads, h3PointDistM, h3PointDistKm. And make h3GetRes0Indexes return constant.

src/Functions/h3PointDistRads.cpp Outdated Show resolved Hide resolved
src/Functions/h3GetRes0Indexes.cpp Outdated Show resolved Hide resolved
src/Functions/h3GetPentagonIndexes.cpp Show resolved Hide resolved
@kitaisreal
Copy link
Collaborator

@bharatnc could you please provide fixes ?

@bharatnc
Copy link
Contributor Author

@bharatnc could you please provide fixes ?

Yes, I will work on the fixes next.

@bharatnc bharatnc requested a review from kitaisreal February 22, 2022 06:11
@bharatnc
Copy link
Contributor Author

@bharatnc could you please provide fixes ?

Yes, I will work on the fixes next.

@kitaisreal I've made those changes, PR is ready for another round of reviews.


auto data = ColumnArray::create(ColumnUInt64::create());
/// res0_indexes contains all indexes with resolution 0 as returned by the h3 getRes0Cells function
const Array res0_indexes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, we just need to create ColumnArray, then create ColumnUInt64, and in ColumnUInt64 pass result of h3 function call.
Current implementation will always allocate big array of Field type, and writing such constants manual by hand are error-prone.

Copy link
Contributor Author

@bharatnc bharatnc Feb 23, 2022

Choose a reason for hiding this comment

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

I've changed to remove hard coded constant.
However, I need some help to figure out how to pass results of h3 func to ColumnUInt64 directly without copying the results into an intermediate Array and then inserting that into ColumnArray. When I tried to do return ColumnArray::create(std::move(res)) I wasn't able to do it without the offsets i.e it looks like ColumnArray::create expects offsets along with the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kitaisreal can you take another look here. Would be nice to have some pointers to instantiate the result of h3 function call directly to ColumnArray without having to deal with offsets or intermediate array.

@bharatnc bharatnc force-pushed the ncb/h3-misc-funcs-3 branch from efa8b1e to c0fbf32 Compare February 23, 2022 09:22
@bharatnc bharatnc force-pushed the ncb/h3-misc-funcs-3 branch from c0fbf32 to 9b770a9 Compare February 23, 2022 09:27
@kitaisreal kitaisreal merged commit 9e88f3b into ClickHouse:master Mar 17, 2022
This was referenced Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants