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

support s2 index params #3396

Merged
merged 6 commits into from
Dec 22, 2021
Merged

Conversation

jievince
Copy link
Contributor

@jievince jievince commented Dec 2, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

Support the syntax like:

create tag index geo_index on any_shape(geo) s2_max_level=28, s2_max_cells=16

Which issue(s)/PR(s) this PR relates to?

close #3197

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe: `

@jievince jievince added the incompatible PR: incompatible with the recently released version label Dec 2, 2021
@jievince jievince force-pushed the s2-index-params branch 4 times, most recently from de49438 to ae485d5 Compare December 6, 2021 05:37
@jievince jievince added ready-for-testing PR: ready for the CI test and removed incompatible PR: incompatible with the recently released version labels Dec 6, 2021
@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Dec 7, 2021
@jievince jievince removed the ready-for-testing PR: ready for the CI test label Dec 7, 2021
@jievince jievince force-pushed the s2-index-params branch 2 times, most recently from 1944639 to 064ed80 Compare December 8, 2021 06:39
@jievince jievince added the ready-for-testing PR: ready for the CI test label Dec 8, 2021
@jievince jievince force-pushed the s2-index-params branch 4 times, most recently from e0dab64 to bc06761 Compare December 9, 2021 06:33
@jievince jievince requested review from yixinglu and czpmango December 9, 2021 06:36
@jievince jievince changed the title [WIP] support s2 index params support s2 index params Dec 9, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Merging #3396 (33b029b) into master (53b85dc) will decrease coverage by 0.01%.
The diff coverage is 85.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
- Coverage   85.19%   85.18%   -0.02%     
==========================================
  Files        1306     1307       +1     
  Lines      122158   122550     +392     
==========================================
+ Hits       104078   104397     +319     
- Misses      18080    18153      +73     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.h 95.65% <ø> (ø)
src/common/expression/VariableExpression.cpp 84.84% <ø> (-0.45%) ⬇️
src/common/geo/GeoIndex.h 100.00% <ø> (ø)
src/graph/executor/maintain/EdgeIndexExecutor.cpp 92.06% <ø> (ø)
src/graph/executor/maintain/TagIndexExecutor.cpp 92.80% <ø> (ø)
...rc/graph/executor/query/AppendVerticesExecutor.cpp 98.30% <ø> (ø)
src/graph/executor/query/GetEdgesExecutor.cpp 94.00% <ø> (ø)
src/graph/executor/query/GetVerticesExecutor.cpp 96.77% <ø> (ø)
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <ø> (ø)
src/graph/optimizer/rule/IndexScanRule.h 100.00% <ø> (ø)
... and 101 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 6b801bd...33b029b. Read the comment docs.

@jievince jievince requested a review from Shylock-Hg December 9, 2021 07:31
;

index_param_item
: comment_prop_assignment {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment_prop is better than comment_prop_assignment, refer to https://dev.mysql.com/doc/refman/8.0/en/create-index.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shylock-Hg WDYT ?

Copy link
Contributor

@Shylock-Hg Shylock-Hg Dec 20, 2021

Choose a reason for hiding this comment

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

It'ok. But better to modify it in independent PR.

tests/tck/features/geo/GeoBase.feature Outdated Show resolved Hide resolved
src/parser/parser.yy Outdated Show resolved Hide resolved
@jievince jievince added the incompatible PR: incompatible with the recently released version label Dec 16, 2021
@jievince jievince requested review from yixinglu and Shylock-Hg and removed request for Shylock-Hg December 20, 2021 05:43
@jievince jievince mentioned this pull request Dec 20, 2021
7 tasks
src/interface/meta.thrift Outdated Show resolved Hide resolved
tests/tck/features/geo/GeoBase.feature Outdated Show resolved Hide resolved
@jievince jievince force-pushed the s2-index-params branch 8 times, most recently from 4f53ea7 to 267f874 Compare December 21, 2021 15:10
yixinglu
yixinglu previously approved these changes Dec 22, 2021
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

@yixinglu yixinglu merged commit e0e7f45 into vesoft-inc:master Dec 22, 2021
@jievince jievince deleted the s2-index-params branch December 22, 2021 04:26
@cooper-lzy cooper-lzy self-requested a review January 5, 2022 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation incompatible PR: incompatible with the recently released version ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geo Spatial: support to specify s2 region coverer params when create geo index
6 participants