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

ignore existed index #3392

Merged
merged 4 commits into from
Dec 16, 2021
Merged

ignore existed index #3392

merged 4 commits into from
Dec 16, 2021

Conversation

critical27
Copy link
Contributor

@critical27 critical27 commented Dec 2, 2021 •
edited
Loading

Mainly for POC, same as 1.0. If specified with IGNORE_EXISTED_INDEX, we won't read or delete old index when insert. It could be useful when we do the first import, or the index is rarely modified.

Pros: no more read old value or delete old index when we insert, only write will be involved, same as no index, performance will get a great improvement.
Cons: since we don't delete index, lookup will read some wrong data. User should really be careful about this

For example :

INSERT VERTEX person(id) VALUES "100":(1), "200":(1)

LOOKUP ON person WHERE person.id == 1 YIELD id(vertex) as id
// both 100 and 200 will satisfy

INSERT VERTEX IGNORE_EXISTED_INDEX person(id) VALUES "200":(2)

LOOKUP ON person WHERE person.id == 1 YIELD id(vertex) as id
// both 100 and 200 will satisfy, since old index of 200 is not deleted

LOOKUP ON person WHERE person.id == 2 YIELD id(vertex) as id
// only 200 will satisfy

Documents may or may not add this IGNORE_EXISTED_INDEX (up to you guys), if it is misused, wrong data will be looked up.

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Dec 2, 2021
@critical27 critical27 added the doc affected PR: improvements or additions to documentation label Dec 2, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021 •
edited
Loading

Codecov Report

Merging #3392 (008dde3) into master (99f1f7a) will increase coverage by 0.02%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3392      +/-   ##
==========================================
+ Coverage   85.19%   85.21%   +0.02%     
==========================================
  Files        1304     1304              
  Lines      121308   121347      +39     
==========================================
+ Hits       103352   103410      +58     
+ Misses      17956    17937      -19
Impacted Files Coverage Δ
src/clients/storage/StorageClient.h 100.00% <ø> (ø)
src/clients/storage/StorageClientBase-inl.h 74.61% <ø> (+1.93%) ⬆️
src/graph/validator/MutateValidator.h 100.00% <ø> (ø)
src/storage/mutate/AddEdgesProcessor.h 100.00% <ø> (ø)
src/storage/mutate/AddVerticesProcessor.h 100.00% <ø> (ø)
src/storage/mutate/AddEdgesProcessor.cpp 57.23% <78.57%> (+0.57%) ⬆️
src/clients/storage/StorageClient.cpp 78.51% <100.00%> (+0.07%) ⬆️
src/graph/executor/mutate/InsertExecutor.cpp 100.00% <100.00%> (ø)
src/graph/planner/plan/Mutate.h 100.00% <100.00%> (ø)
src/graph/validator/MutateValidator.cpp 94.17% <100.00%> (+0.06%) ⬆️
... and 22 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 75f1112...008dde3. Read the comment docs.

@HarrisChu
Copy link
Contributor

LOOKUP ON person WHERE person.id == 1 YIELD id(vertex) as id
// only 200 will satisfy

person.id == 2?

@HarrisChu
Copy link
Contributor

after this, when running rebuild index, what's the behavior for "200" vertext?

@critical27
Copy link
Contributor Author

LOOKUP ON person WHERE person.id == 1 YIELD id(vertex) as id // only 200 will satisfy

person.id == 2?

My bad, 👍

@critical27
Copy link
Contributor Author

person.id == 1

Both person.id == 1 and person.id == 2 will satisfy

@critical27
Copy link
Contributor Author

critical27 commented Dec 3, 2021 •
edited
Loading

There are memory corruption, hold on one second.

Ready now

panda-sheep
panda-sheep approved these changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@panda-sheep panda-sheep 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!

wenhaocs
wenhaocs approved these changes Dec 16, 2021
View reviewed changes
Copy link
Contributor

@wenhaocs wenhaocs left a comment

Choose a reason for hiding this comment

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

Nice and clean. Great job.

yixinglu
yixinglu reviewed Dec 16, 2021
View reviewed changes
src/parser/parser.yy
@@ -162,7 +162,7 @@ static constexpr size_t kCommentLengthLimit = 256;
%token KW_BOOL KW_INT8 KW_INT16 KW_INT32 KW_INT64 KW_INT KW_FLOAT KW_DOUBLE
%token KW_STRING KW_FIXED_STRING KW_TIMESTAMP KW_DATE KW_TIME KW_DATETIME
%token KW_GO KW_AS KW_TO KW_USE KW_SET KW_FROM KW_WHERE KW_ALTER
%token KW_MATCH KW_INSERT KW_VALUE KW_VALUES KW_YIELD KW_RETURN KW_CREATE KW_VERTEX KW_VERTICES
%token KW_MATCH KW_INSERT KW_VALUE KW_VALUES KW_YIELD KW_RETURN KW_CREATE KW_VERTEX KW_VERTICES KW_IGNORE_EXISTED_INDEX
Copy link
Contributor

@yixinglu yixinglu Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

emm.. This IGNORE_EXISTED_INDEX keyword is a little complicated to use, maybe it's better to split it into some words such as:

INSERT VERTEX IGNORE EXISTS INDEX ...

reference mysql insert ignore statement.

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 understand, but this is for almost the only one poc. They have used it since 1.X... Maybe just keep it.

@critical27 critical27 merged commit f14aa0e into vesoft-inc:master Dec 16, 2021
@critical27 critical27 deleted the ignore branch December 16, 2021 08:29
@jamieliu1023 jamieliu1023 mentioned this pull request Dec 18, 2021
Closed
@cooper-lzy cooper-lzy requested a review from foesa-yang January 5, 2022 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yixinglu yixinglu yixinglu left review comments

@panda-sheep panda-sheep panda-sheep approved these changes

@wenhaocs wenhaocs wenhaocs approved these changes

@darionyaphet darionyaphet Awaiting requested review from darionyaphet

@Nivras Nivras Awaiting requested review from Nivras

@cangfengzhs cangfengzhs Awaiting requested review from cangfengzhs

@liwenhui-soul liwenhui-soul Awaiting requested review from liwenhui-soul

@liuyu85cn liuyu85cn Awaiting requested review from liuyu85cn

@SuperYoko SuperYoko Awaiting requested review from SuperYoko

@pengweisong pengweisong Awaiting requested review from pengweisong

@foesa-yang foesa-yang Awaiting requested review from foesa-yang

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
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@critical27 @codecov-commenter @HarrisChu @yixinglu @wenhaocs @panda-sheep