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.

Sign up for GitHub

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

Jump to bottom

Geo Spatial: 1. geography common #2954

Merged
merged 9 commits into from
Sep 29, 2021
Merged

Geo Spatial: 1. geography common #2954

merged 9 commits into from
Sep 29, 2021

Conversation

jievince
Copy link
Contributor

@jievince jievince commented Sep 27, 2021 •
edited by Sophie-Xie
Loading

This pr has done the followings:

  1. add geography value;
  2. add wkt/wkb parser(reader)/writer

Some extra notes:
Our macro MUST_USE_RESULT conflicts with s2's third-party's macro, so I change it to NG_MUST_USE_RESULT

@jievince jievince added the ready-for-testing PR: ready for the CI test label Sep 27, 2021
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 27, 2021
@jievince jievince requested a review from Aiee September 28, 2021 01:55
@jievince jievince force-pushed the geo-spatial-common branch 2 times, most recently from f0d2ce3 to 27a3283 Compare September 28, 2021 02:34
yixinglu
yixinglu reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

Good start.

src/codec/RowWriterV2.cpp Show resolved Hide resolved
src/common/datatypes/Geography.cpp Outdated Show resolved Hide resolved
src/common/datatypes/Geography.cpp Show resolved Hide resolved
src/common/datatypes/Geography.cpp Outdated Show resolved Hide resolved
src/common/datatypes/Geography.cpp Outdated Show resolved Hide resolved
src/common/datatypes/Value.cpp Outdated Show resolved Hide resolved
src/common/geo/GeoUtils.h Outdated Show resolved Hide resolved
src/common/geo/GeoUtils.h Outdated Show resolved Hide resolved
src/common/geo/io/wkt/WKTScanner.h Show resolved Hide resolved
src/common/geo/io/wkt/wkt_parser.yy Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2021 •
edited
Loading

Codecov Report

Merging #2954 (ad28e3e) into master (dddf54e) will decrease coverage by 1.05%.
The diff coverage is 66.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2954      +/-   ##
==========================================
- Coverage   85.47%   84.42%   -1.06%     
==========================================
  Files        1229     1273      +44     
  Lines      110375   112914    +2539     
==========================================
+ Hits        94341    95323     +982     
- Misses      16034    17591    +1557
Impacted Files Coverage Δ
src/clients/meta/MetaClient.h 92.30% <ø> (ø)
src/codec/RowReaderV2.cpp 95.32% <0.00%> (-0.90%) ⬇️
src/codec/test/SchemaWriter.cpp 92.18% <0.00%> (-4.54%) ⬇️
src/common/conf/Configuration.h 100.00% <ø> (ø)
src/common/geo/GeoUtils.h 0.00% <0.00%> (ø)
src/common/meta/NebulaSchemaProvider.cpp 87.37% <0.00%> (-1.74%) ⬇️
src/common/thread/GenericWorker.h 96.15% <ø> (+3.84%) ⬆️
src/common/utils/NebulaKeyUtils.cpp 89.03% <0.00%> (-4.89%) ⬇️
src/common/utils/NebulaKeyUtils.h 80.68% <ø> (+2.65%) ⬆️
src/graph/optimizer/OptimizerUtils.cpp 83.99% <ø> (ø)
... and 252 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 8546f20...ad28e3e. Read the comment docs.

critical27
critical27 reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Generally LGTM, good job, see the comments inline.

src/common/geo/io/wkb/ByteOrder.h Show resolved Hide resolved
src/common/geo/io/Geometry.h Outdated
Coordinate coord;

explicit Point(const Coordinate &v) : coord(v) {}
explicit Point(Coordinate &&v) : coord(std::move(v)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, if you define copy/move constructor, consider provide copy/move assignment as well

Check these class in Geometry.h

src/common/geo/GeoUtils.h Show resolved Hide resolved
src/common/geo/io/wkb/WKBReader.cpp Outdated Show resolved Hide resolved
src/common/geo/io/wkt/WKTWriter.cpp Outdated Show resolved Hide resolved
@jievince jievince force-pushed the geo-spatial-common branch 3 times, most recently from 06f0998 to a3d8297 Compare September 28, 2021 13:33
@jievince jievince changed the title Geo Spatial: add geograpy data Geo Spatial: 1. geograpy data Sep 29, 2021
yixinglu
yixinglu previously approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

LGTM

czpmango
czpmango reviewed Sep 29, 2021
View reviewed changes
src/common/utils/IndexKeyUtils.h
Comment on lines +226 to +230
auto* c1 = reinterpret_cast<const char*>(&val);
auto i = *reinterpret_cast<const int64_t*>(c1);
i = -(std::numeric_limits<int64_t>::max() + i);
val = *reinterpret_cast<const double*>(&i);
auto* c2 = reinterpret_cast<const char*>(&i);
val = *reinterpret_cast<const double*>(c2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain it.

Copy link
Contributor Author

@jievince jievince Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Have a look at https://stackoverflow.com/questions/4163126/dereferencing-type-punned-pointer-will-break-strict-aliasing-rules-warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find nebula has configured the option:

option(ENABLE_STRICT_ALIASING           "Build with -fstrict-aliasing" OFF)

if(NOT ENABLE_STRICT_ALIASING)
    add_compile_options(-fno-strict-aliasing)
else()
    add_compile_options(-fstrict-aliasing)
endif()

I don't know why this option doesn't work now

czpmango
czpmango previously approved these changes Sep 29, 2021
View reviewed changes
@yixinglu yixinglu merged commit 2c3e790 into vesoft-inc:master Sep 29, 2021
@jievince jievince changed the title Geo Spatial: 1. geograpy data Geo Spatial: 1. Add geograpy value Sep 29, 2021
wenhaocs added a commit to wenhaocs/nebula that referenced this pull request Sep 30, 2021
@wenhaocs
Revert "Geo Spatial: 1. geograpy data (vesoft-inc#2954)"
8b451dc 
This reverts commit 2c3e790.
@wenhaocs wenhaocs mentioned this pull request Oct 1, 2021
Closed
@jamieliu1023 jamieliu1023 mentioned this pull request Oct 2, 2021
Closed
@jievince jievince changed the title Geo Spatial: 1. Add geograpy value Geo Spatial: 1. geograpy data Oct 4, 2021
@jievince jievince changed the title Geo Spatial: 1. geograpy data Geo Spatial: 1. geograpy common Oct 9, 2021
@jievince jievince mentioned this pull request Oct 12, 2021
Closed
@jievince jievince deleted the geo-spatial-common branch October 18, 2021 07:08
@jievince jievince changed the title Geo Spatial: 1. geograpy common Geo Spatial: 1. geography common Oct 19, 2021
@jievince jievince added the doc affected PR: improvements or additions to documentation label Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shylock-Hg Shylock-Hg Shylock-Hg left review comments

@Aiee Aiee Aiee approved these changes

@czpmango czpmango czpmango approved these changes

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

@critical27 critical27 Awaiting requested review from critical27 critical27 is a code owner

@yixinglu yixinglu Awaiting requested review from yixinglu

Assignees
No one assigned
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jievince @codecov-commenter @yixinglu @critical27 @Aiee @czpmango @Shylock-Hg @Sophie-Xie