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

Rollback vertex key #4629

Merged
merged 11 commits into from
Sep 21, 2022
Merged

Rollback vertex key #4629

merged 11 commits into from
Sep 21, 2022

Conversation

critical27
Copy link
Contributor

@critical27 critical27 commented Sep 8, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

Rollback the vertex key, upgrade tool is removed directly.

There are still two tck cases I think it is wrong:

  1. GoYieldVertexEdge.feature: multi edges over all
  2. Go.feature: multi edges over all

How do you solve it?

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

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

@critical27 critical27 added the do not review PR: not ready for the code review yet label Sep 8, 2022
@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Sep 8, 2022
@Sophie-Xie Sophie-Xie added the ready-for-testing PR: ready for the CI test label Sep 14, 2022
@nevermore3
Copy link
Contributor

nevermore3 commented Sep 20, 2022

When executing query:
    """
    GO FROM "Paul Gasol" OVER *
    WHERE $$.player.name IS NOT NULL
    YIELD edge as e
    """
  Then the result should be, in any order, with relax comparison:
    | e                                                     |
    | [:like "Paul Gasol"->"Kobe Bryant" @0 {likeness: 90}] |
    | [:like "Paul Gasol"->"Marc Gasol" @0 {likeness: 99}]  |

Since getProp previously returned EMPTY rows, now it return nothing, should use is NULL as filter conditions

i already fix the test case

nevermore3
nevermore3 previously approved these changes Sep 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.73%. Comparing base (7d5e93f) to head (9858328).
Report is 471 commits behind head on master.

Files with missing lines Patch % Lines
src/storage/exec/GetPropNode.h 11.11% 8 Missing ⚠️
src/storage/CompactionFilter.h 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4629      +/-   ##
==========================================
+ Coverage   84.67%   84.73%   +0.05%     
==========================================
  Files        1358     1358              
  Lines      135654   135647       -7     
==========================================
+ Hits       114868   114934      +66     
+ Misses      20786    20713      -73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sophie-Xie Sophie-Xie merged commit 545137a into vesoft-inc:master Sep 21, 2022
critical27 added a commit to critical27/nebula that referenced this pull request Oct 12, 2022
critical27 added a commit to critical27/nebula that referenced this pull request Oct 12, 2022
critical27 added a commit that referenced this pull request Oct 13, 2022
* Revert "insert vertex key when only vertex and flag is set (#4685)"

This reverts commit fff82a6.

* Revert "only write vertex key when flag is set or explictly insert vertex (#4680)"

This reverts commit a5bed33.

* Revert "fix bug (#4675)"

This reverts commit cccc014.

* Revert "Rollback vertex key (#4629)"

This reverts commit 545137a.

* wtf

* tck format
Sophie-Xie pushed a commit that referenced this pull request Oct 13, 2022
* Revert "insert vertex key when only vertex and flag is set (#4685)"

This reverts commit fff82a6.

* Revert "only write vertex key when flag is set or explictly insert vertex (#4680)"

This reverts commit a5bed33.

* Revert "fix bug (#4675)"

This reverts commit cccc014.

* Revert "Rollback vertex key (#4629)"

This reverts commit 545137a.

* wtf

* tck format
Sophie-Xie added a commit that referenced this pull request Oct 13, 2022
* Use vertex key only when use_vertex_key is on (#4716)

* Revert "insert vertex key when only vertex and flag is set (#4685)"

This reverts commit fff82a6.

* Revert "only write vertex key when flag is set or explictly insert vertex (#4680)"

This reverts commit a5bed33.

* Revert "fix bug (#4675)"

This reverts commit cccc014.

* Revert "Rollback vertex key (#4629)"

This reverts commit 545137a.

* wtf

* tck format

* fix job status when all finished (#4717)

* fix job name from zone balance to data balance

* address wenhui's comment

* fix create index if not exists behavior (#4705)

Co-authored-by: Sophie <[email protected]>
Co-authored-by: Doodle <[email protected]>

Co-authored-by: Doodle <[email protected]>
Co-authored-by: Alex Xing <[email protected]>
Co-authored-by: kyle.cao <[email protected]>
@foesa-yang
Copy link

@critical27 critical27 deleted the vertex branch November 11, 2022 02:31
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Jan 31, 2023
<!--
Thanks for your contribution!
In order to review PR more efficiently, please add information according to the template.
-->

## What type of PR is this?
- [ ] bug
- [ ] feature
- [X] enhancement

## What problem(s) does this PR solve?
#### Issue(s) number: 

#### Description:
PRD of vertex key changed again... Here is the new version of logic:
1. No matter use_vertex_key is on or off, the query result should be consistent with 3.1.
2. If use_vertex_key is on, we will use the vertex key to detect whether the vertex exists, which is the logic in 3.X version.
3. If use_vertex_key is off, we will prefix scan by vertex to detect whether the vertex exists, **which will bring performance regression**
4. We will insert the vertex only when use_vertex_key is on

> By default, use_vertex_key is off

## How do you solve it?
This PR reverts all related changes before (vesoft-inc#4685 vesoft-inc#4680 vesoft-inc#4675 vesoft-inc#4629), and apply the logic above. 

Related changes:
1. So the upgrade tool is necessary again, so bring it back.
2. The TCK cases keeps same as before vesoft-inc#4629


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

> Maybe review the last commit is enough 


## Checklist:
Tests:
- [X] Unit test(positive and negative cases)
- [ ] Function test
- [ ] Performance test
- [ ] N/A

Affects:
- [X] Documentation affected (Please add the label if documentation needs to be modified.)
- [ ] Incompatibility (If it breaks the compatibility, please describe it and add the label.)
- [ ] If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
- [ ] Performance impacted: Consumes more CPU/Memory


## Release notes:

Please confirm whether to be reflected in release notes and how to describe:
> ex. Fixed the bug .....


Migrated from vesoft-inc#4716

Co-authored-by: Doodle <[email protected]>
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 ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants