Geo spatial: 2. geography functions #2979

jievince · 2021-09-29T16:00:43Z

For the definition of the geo function prefixed with ST, see the following documentation:

codecov-commenter · 2021-10-11T08:26:39Z

Codecov Report

Merging #2979 (8beac94) into master (e642c05) will decrease coverage by 0.14%.
The diff coverage is 71.79%.

@@            Coverage Diff             @@
##           master    #2979      +/-   ##
==========================================
- Coverage   84.34%   84.20%   -0.15%     
==========================================
  Files        1283     1296      +13     
  Lines      113684   115342    +1658     
==========================================
+ Hits        95891    97122    +1231     
- Misses      17793    18220     +427

Impacted Files	Coverage Δ
src/common/geo/io/wkb/WKBReader.h	`100.00% <ø> (ø)`
src/common/geo/io/wkb/WKBWriter.h	`100.00% <ø> (ø)`
src/common/geo/io/wkt/WKTWriter.h	`100.00% <ø> (ø)`
src/graph/visitor/DeduceTypeVisitor.h	`100.00% <ø> (ø)`
src/storage/admin/AdminTaskManager.h	`100.00% <ø> (ø)`
src/storage/admin/AdminTaskProcessor.cpp	`0.00% <0.00%> (ø)`
src/common/geo/function/S2Util.cpp	`18.18% <18.18%> (ø)`
src/storage/admin/AdminTaskManager.cpp	`49.55% <34.28%> (-1.18%)`	⬇️
src/common/function/FunctionManager.cpp	`75.42% <35.29%> (-4.98%)`	⬇️
src/common/datatypes/GeographyOps-inl.h	`37.06% <37.94%> (+4.37%)`	⬆️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64721f1...8beac94. Read the comment docs.

src/common/datatypes/Geography.cpp

yixinglu · 2021-10-11T15:43:06Z

src/common/datatypes/Geography.cpp

+
+bool Coordinate::isValid() const { return std::abs(x) <= 180.0 && std::abs(y) <= 90.0; }
+
+void Point::normalize() {}


coord.normalize() ?

The method Coordinate::normalize is not used now.
For the illegal coordinate, we want to report an error directly, rather than normalize it for the user

src/common/datatypes/Geography.cpp

yixinglu · 2021-10-11T15:48:25Z

src/common/datatypes/Geography.h

+};
+
+struct Point {
+  Coordinate coord;


why not use x,y here directly, do we really need the coordinate struct ?

There are two modeling approaches:
1:
Point: Coordinate;
LineString: A list of Coordinate;
Polygon: Lists of list of Coordinate;
2:
Point;
LineString: A list of Point;
Polygon: Lists of LineString.

Both are ok, but almost all geo libraries take the first approach

src/common/geo/function/Intersects.h

CPWstatic

Excellent!

CPWstatic · 2021-10-09T02:34:21Z

src/common/datatypes/Geography.cpp

+  switch (shape()) {
+    case GeoShape::POINT: {
+      const auto& point = geom->point();
+      return std::abs(point.coord.x) <= 180.0 && std::abs(point.coord.y) <= 90.0;


Consider a macro or constexpr for representing the magic number 180.0 and 90.0.

src/common/datatypes/Geography.cpp

CPWstatic · 2021-10-12T04:05:38Z

src/common/datatypes/Geography.h

+  void __clear() { clear(); }
+
+  // TODO(jie) compare double correctly
+  bool operator==(const Coordinate& rhs) const { return x == rhs.x && y == rhs.y; }


abs(x - rhs) < eps

CPWstatic · 2021-10-12T05:03:19Z

src/common/function/FunctionManager.cpp

@@ -302,6 +308,94 @@ std::unordered_map<std::string, std::vector<TypeSignature>> FunctionManager::typ
         TypeSignature({Value::Type::DATASET, Value::Type::INT, Value::Type::STRING},
                       Value::Type::__EMPTY__),
     }},
+    // These geo functions of the ST prefix follow the Simple Feature Access and SQL/MM


What does ST means?

The SQL/MM standard uses consistently the prefix ST for all tables, views, types, methods, and function names. The prefix stood originally for Spatial and Temporal. It was intended in the early stages of the standard development to define a combination of temporal and spatial extension.
See https://stackoverflow.com/questions/7234679/what-is-st-in-postgis

Almost all SQL databases use this standard when implementing geo spatial.

yixinglu

LGTM

critical27

Generally LGTM

* Geo spatial: 2. Add geo functions * add ByteOrderDataIOStream * add s2 util test * let Geography store variant * address yee's comments

jievince changed the title ~~Geo spatial: 2. Add geo functions~~ Geo spatial: 2. Add geography functions Sep 29, 2021

jievince added the wip Solution: work in progress label Sep 29, 2021

Sophie-Xie added this to the v2.6.0 milestone Sep 29, 2021

Sophie-Xie requested review from critical27 and nevermore3 September 30, 2021 01:34

jievince force-pushed the geo-spatial-functions branch from 29081e0 to de31c73 Compare October 4, 2021 03:16

jievince changed the title ~~Geo spatial: 2. Add geography functions~~ Geo spatial: 2. geography functions Oct 4, 2021

jievince force-pushed the geo-spatial-functions branch 7 times, most recently from 4d52a75 to 340401e Compare October 8, 2021 03:35

jievince added the cherry-pick-v2.6 PR: need cherry-pick to this version label Oct 8, 2021

jievince force-pushed the geo-spatial-functions branch 4 times, most recently from 1adae70 to 5e9d5c5 Compare October 10, 2021 08:22

jievince added ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Oct 10, 2021

jievince force-pushed the geo-spatial-functions branch 5 times, most recently from 996532c to 53b8b2b Compare October 11, 2021 06:06

jievince requested review from czpmango, CPWstatic and yixinglu October 11, 2021 06:11

jievince force-pushed the geo-spatial-functions branch from 53b8b2b to c1188d7 Compare October 11, 2021 06:12

jievince force-pushed the geo-spatial-functions branch from c1188d7 to d56facc Compare October 11, 2021 06:17

jievince added 3 commits October 11, 2021 14:48

Geo spatial: 2. Add geo functions

0093d1b

add ByteOrderDataIOStream

c206a0d

add s2 util test

cbcbb07

jievince force-pushed the geo-spatial-functions branch from d56facc to f5b54c6 Compare October 11, 2021 06:53

let Geography store variant

8beac94

jievince force-pushed the geo-spatial-functions branch from f5b54c6 to 8beac94 Compare October 11, 2021 06:58

yixinglu reviewed Oct 11, 2021

View reviewed changes

address yee's comments

bc17814

jievince requested review from Shylock-Hg and yixinglu October 12, 2021 03:48

CPWstatic reviewed Oct 12, 2021

View reviewed changes

yixinglu approved these changes Oct 12, 2021

View reviewed changes

critical27 approved these changes Oct 12, 2021

View reviewed changes

jievince requested a review from yixinglu October 12, 2021 07:43

critical27 merged commit d8e5539 into vesoft-inc:master Oct 12, 2021

Sophie-Xie pushed a commit that referenced this pull request Oct 12, 2021

Geo spatial: 2. geography functions (#2979)

be21aee

* Geo spatial: 2. Add geo functions * add ByteOrderDataIOStream * add s2 util test * let Geography store variant * address yee's comments

jievince mentioned this pull request Oct 12, 2021

Geo spatial: Geography #2830

Closed

jievince deleted the geo-spatial-functions branch October 13, 2021 01:56

jievince mentioned this pull request Oct 19, 2021

Geo spatial: 3. geography schema, data, index and optimization #3043

Merged

jievince added the doc affected PR: improvements or additions to documentation label Oct 20, 2021

jamieliu1023 mentioned this pull request Nov 1, 2021

Weekly Report 2021-10-17 vesoft-inc/nebula-community#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geo spatial: 2. geography functions #2979

Geo spatial: 2. geography functions #2979

jievince commented Sep 29, 2021 •

edited

codecov-commenter commented Oct 11, 2021

yixinglu Oct 11, 2021

jievince Oct 12, 2021

yixinglu Oct 11, 2021

jievince Oct 12, 2021

CPWstatic left a comment •

edited

CPWstatic Oct 9, 2021

jievince Oct 12, 2021

CPWstatic Oct 12, 2021

jievince Oct 12, 2021

CPWstatic Oct 12, 2021

jievince Oct 12, 2021

jievince Oct 12, 2021

yixinglu left a comment

critical27 left a comment


		bool Coordinate::isValid() const { return std::abs(x) <= 180.0 && std::abs(y) <= 90.0; }

		void Point::normalize() {}

Geo spatial: 2. geography functions #2979

Geo spatial: 2. geography functions #2979

Conversation

jievince commented Sep 29, 2021 • edited

codecov-commenter commented Oct 11, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPWstatic left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yixinglu left a comment

Choose a reason for hiding this comment

critical27 left a comment

Choose a reason for hiding this comment

jievince commented Sep 29, 2021 •

edited

CPWstatic left a comment •

edited