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

h3 functions required for circle search #8034

Merged
merged 12 commits into from
Dec 16, 2019

Conversation

hombit
Copy link
Contributor

@hombit hombit commented Dec 4, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=ru

Changelog category (leave one):

  • New Feature

Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):

Support of several h3 function in addition to geoToH3:

  • h3GetResolution
  • h3EdgeAngle
  • h3EdgeLength
  • h3IsValid
  • h3kRing

Examples:

CREATE TABLE places
(
    `place` String,
    `lon` Float64,
    `lat` Float64,
    `h3index7` UInt64
)
ENGINE = Memory();

INSERT INTO places VALUES\
    ('Yandex', 37.588144, 55.733842, geoToH3(37.588144, 55.733842, 7)) \
    ('Moscow University',  37.531014, 55.703050, geoToH3(37.531014, 55.703050, 7)) \
    ('Uber',-122.4201594, 37.7756894, geoToH3(-122.4201594, 37.7756894,  7));

SELECT * FROM places WHERE h3index7 = geoToH3(37.542779, 55.701009, 7);
┌─place─────────────┬───────lon─┬──────lat─┬───────────h3index7─┐
│ Moscow University │ 37.531014 │ 55.70305 │ 608296731968274431 │
└───────────────────┴───────────┴──────────┴────────────────────┘
SELECT *
FROM places
WHERE h3index7 IN
(
    WITH
        37.577298 AS lon_,
        55.710832 AS lat_
    SELECT arrayJoin(h3kRing(geoToH3(lon_, lat_, 7), 1))
);
┌─place─────────────┬───────lon─┬───────lat─┬───────────h3index7─┐
│ Yandex            │ 37.588144 │ 55.733842 │ 608296731481735167 │
│ Moscow University │ 37.531014 │  55.70305 │ 608296731968274431 │
└───────────────────┴───────────┴───────────┴────────────────────┘
WITH
    37.577298 AS lon_,
    55.710832 AS lat_,
    3000 AS radius
SELECT *
FROM places
WHERE (h3index7 IN
(
    WITH
        37.577298 AS lon_,
        55.710832 AS lat_,
        3000 AS radius
    SELECT arrayJoin(h3kRing(geoToH3(lon_, lat_, 7), toUInt8(radius / h3EdgeLengthM(7))))
)) AND (greatCircleDistance(lon, lat, lon_, lat_) < radius)
┌─place──┬───────lon─┬───────lat─┬───────────h3index7─┐
│ Yandex │ 37.588144 │ 55.733842 │ 608296731481735167 │
└────────┴───────────┴───────────┴────────────────────┘

Fix #8000

@hombit hombit changed the title Add support of some h3 functions h3 functions required for circle search Dec 5, 2019
@hombit hombit changed the title h3 functions required for circle search h3 functions required by circle search Dec 5, 2019
@hombit hombit changed the title h3 functions required by circle search h3 functions required for circle search Dec 5, 2019
@hombit hombit force-pushed the h3support branch 2 times, most recently from 3e86501 to 508ee53 Compare December 5, 2019 16:03
@hombit hombit marked this pull request as ready for review December 7, 2019 17:15
@hombit hombit force-pushed the h3support branch 2 times, most recently from a8ad932 to c39cf03 Compare December 7, 2019 17:33
{
#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdocumentation"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mark the include directory as SYSTEM in CMakeLists will help.

Copy link
Contributor Author

@hombit hombit Dec 8, 2019

Choose a reason for hiding this comment

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

I just copy-pasted it from geoToH3.cpp, can you explain how should I fix it?

@hombit hombit force-pushed the h3support branch 5 times, most recently from 11cedd1 to 4b60bc9 Compare December 11, 2019 10:04
@hombit
Copy link
Contributor Author

hombit commented Dec 11, 2019

I've rebased it on current master

const int k = col_k->getInt(row);

const auto arr_size = H3_EXPORT(maxKringSize)(k);
std::unique_ptr<H3Index[]> hindex_arr{new H3Index[arr_size]};
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it happen here?

extern "C" {
# ifdef __clang__
# pragma clang diagnostic push
# pragma clang diagnostic ignored "-Wdocumentation"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to do without ignoring this flag? Actually we already have had documentation comment....

Copy link
Contributor Author

@hombit hombit Dec 16, 2019

Choose a reason for hiding this comment

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

Actually, I don't know what it is for, I just copy-pasted it from geoToH3.cpp I'll remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case probably we can't do without it

@alexey-milovidov alexey-milovidov merged commit 12b3dcd into ClickHouse:master Dec 16, 2019
alexey-milovidov added a commit that referenced this pull request Dec 16, 2019
alexey-milovidov added a commit that referenced this pull request Dec 16, 2019
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Dec 16, 2019

0042c82
0bc1c6d

  1. Why I have removed warning suppression of -Wdocumentation for clang?

Because h3 headers are included as system headers:
https://github.com/ClickHouse/ClickHouse/blob/master/contrib/h3-cmake/CMakeLists.txt#L25
Compiler will suppress all warnings from system headers in the same way as for headers from /usr/include.

Why it's ok to suppress all warnings from 3rd party library?
Because we compile our code with much more warnings as usual (-Weverything) and it's natural for 3rd party code to not pass this checks.

  1. Why I have removed extern "C" around include?

Because h3 library already contains #ifdef __cplusplus ... extern "C".

  1. Why I have removed the usage of H3_EXPORT macro?

It is needed only if the user decided to prefix all h3 functions. We didn't do that.

@stavrolia
Copy link
Contributor

Why don't we want to prefix C function from h3?

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Dec 16, 2019

Why don't we want to prefix C function from h3?

We don't need that - there is no name collisions.

@vitlibar vitlibar added the pr-feature Pull request with new product feature label Dec 23, 2019
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.

Add full support of h3 indices
4 participants