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

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

Merged
merged 3 commits into from
Oct 20, 2021

Conversation

jievince
Copy link
Contributor

@jievince jievince commented Oct 12, 2021

Close #2595

TODO:

  • Support to specify s2 region covering options when create index on geo column:
    CREATE TAG INDEX geo_index on shape(geo) s2_min_level = 0, s2_max_level=30, s2_max_cells=8
  • Support joint index for geography column with other columns.
    The joint index could only have one geography field and it must be at the end.
    (For the time reason, we only support create a single index on a geography column)
  • Support geo predicate index optimization for MATCH. I have only done it for LOOKUP currently.
    This could be done after the index selection and index optimization are done.
    (For MATCH, geo predicate index will not apply to its where clause currently, it will degenerate to IndexFullScan + Filter;)
  • Try to find the best default value for s2 region cover options.
  • Improve GeoIndex::coveredBy

@jievince jievince added wip Solution: work in progress cherry-pick-v2.6 PR: need cherry-pick to this version labels Oct 12, 2021
@jievince jievince force-pushed the geo-spatial-data-and-index branch 2 times, most recently from 9306497 to 8aeedd9 Compare October 12, 2021 09:37
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Oct 12, 2021
@jievince jievince mentioned this pull request Oct 12, 2021
@jievince jievince force-pushed the geo-spatial-data-and-index branch 7 times, most recently from aaa81ca to e30c3f5 Compare October 14, 2021 01:45
@jievince jievince changed the title Geo spatial: 3. geography data and index Geo spatial: 3. geography schema, data and index Oct 14, 2021
@jievince jievince force-pushed the geo-spatial-data-and-index branch 10 times, most recently from 00a2d27 to 5ff3dbc Compare October 15, 2021 07:38
@jievince jievince changed the title Geo spatial: 3. geography schema, data and index Geo spatial: 3. geography schema, data, index and optimization Oct 15, 2021
@jievince jievince force-pushed the geo-spatial-data-and-index branch 5 times, most recently from 66d0471 to 3354f42 Compare October 18, 2021 03:43
@jievince jievince force-pushed the geo-spatial-data-and-index branch from 68e597d to 7b1b40a Compare October 19, 2021 12:35
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.

I have reviewed the storage part, it is ok to return std::vector for vertexIndexKeys and edgeIndexKeys, maybe some perf regression, but it is ok.

DCHECK(!hasGeo || cols.size() == 1);
std::vector<std::string> indexes;

if (!hasGeo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a LIKELY here.

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 think it might not be good to add LIKELY here, because there is no way to know the user scenario, for example, the user may use nebula as a geospatial database, so all the indexes are geography indexes

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@jievince jievince force-pushed the geo-spatial-data-and-index branch from 7b1b40a to a1899bc Compare October 19, 2021 12:45
@jievince
Copy link
Contributor Author

jievince commented Oct 19, 2021

I have reviewed the storage part, it is ok to return std::vector for vertexIndexKeys and edgeIndexKeys, maybe some perf regression, but it is ok.

When I first implemented it, I did implement it separately: return vector for geography, return a single string for other datatypes. But I feel it makes the code less readable, and when we later support joint index of geo with other datatypes, which will also need to return a vector, this will lead to increased code complexity. So I change it to make them all return vector.

There may be a bit of performance overhead to return a vector for all, we could do some perf regression. @FangJinhe @Sophie-Xie

@jievince jievince force-pushed the geo-spatial-data-and-index branch from a1899bc to bc45f3f Compare October 19, 2021 13:19
src/tools/db-upgrade/DbUpgrader.cpp Outdated Show resolved Hide resolved
src/tools/db-upgrade/DbUpgrader.cpp Outdated Show resolved Hide resolved
@jievince jievince force-pushed the geo-spatial-data-and-index branch from bc45f3f to 4674a48 Compare October 19, 2021 13:32
yixinglu
yixinglu previously approved these changes Oct 20, 2021
@jievince jievince force-pushed the geo-spatial-data-and-index branch from 2940bdc to fee8db2 Compare October 20, 2021 03:48
@jievince jievince force-pushed the geo-spatial-data-and-index branch from 4b6f4bf to a4afcc7 Compare October 20, 2021 05:06
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.

Good job~

@critical27 critical27 merged commit e179fc4 into vesoft-inc:master Oct 20, 2021
@jievince jievince deleted the geo-spatial-data-and-index branch October 20, 2021 06:10
Sophie-Xie pushed a commit that referenced this pull request Oct 20, 2021
bright-starry-sky pushed a commit that referenced this pull request Oct 20, 2021
* Fixed an issue where the server still started with a wrong ip/host (#3057)

* Fix wrong local ip.

* Address comment.

* Fix alter drop (#3036)

* disable modify same col

* add test case

* refactor ddl

* fix pytest error

* address comment

Co-authored-by: Yee <[email protected]>

* fix fetch vertex properties(vertex) bug (#3120)

* fix fetch vertex properties(vertex) bug

* address comment

Co-authored-by: Yee <[email protected]>

* fix go yield bug (#3128)

* fix go yield bug

* add test case

* Remove unnecessary check (#3112)

Co-authored-by: cpw <[email protected]>
Co-authored-by: Yee <[email protected]>

* fix go mTOn filter bug (#3144)

* fix go mTOn filter bug

* add test case

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

* Geo spatial: 3. geography data and index

Co-authored-by: cpw <[email protected]>
Co-authored-by: jimingquan <[email protected]>
Co-authored-by: Yee <[email protected]>
Co-authored-by: Yichen Wang <[email protected]>
Co-authored-by: jie.wang <[email protected]>
@jievince jievince added the doc affected PR: improvements or additions to documentation label Oct 20, 2021
@Sophie-Xie Sophie-Xie removed the wip Solution: work in progress label Oct 21, 2021
@ZOUG
Copy link

ZOUG commented Nov 13, 2021

Are multipart geometrical datatypes (MultiPoint, MultiLineString, MultiPolygon) supported at the moment? If not, are there any plans for them?

@jievince
Copy link
Contributor Author

jievince commented Dec 2, 2021

Are multipart geometrical datatypes (MultiPoint, MultiLineString, MultiPolygon) supported at the moment? If not, are there any plans for them?

Hi @ZOUG , Sorry for the late reply. We just support Point, LineString and Polygon currently.
Multi-shapes are not supported yet.
Which scenarios do you want to use these Multi-shape? If there is a real need, we will consider implementing them.

By the way, geo really involves a lot of things. At present, we have only implemented part of the core feature.
If everybody needs other things, such as more complex shapes, or wants to import geo objects from GeoJson, etc.
You are welcome to open an issue to put forward your requirements.

if (static_cast<ListExpression*>(inExpr->right())->size() > 1) {
return TransformResult::noTransform();
} else {
transformedExpr = graph::ExpressionUtils::rewriteInExpr(condition);
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 133 is a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-v2.6 PR: need cherry-pick to this version doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QE]Geo-Code support (Geo data type, index, etc.?)
7 participants