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

format fetchedges #2779

Merged
merged 5 commits into from
Sep 28, 2021
Merged

format fetchedges #2779

merged 5 commits into from
Sep 28, 2021

Conversation

nevermore3
Copy link
Contributor

@nevermore3 nevermore3 commented Sep 1, 2021

1、refactor fetch edges
2、support FETCH PROP ON edgeName "A"->"B" YIELD edge AS e, src(edge), dst(edge), rank(edge), type(edge)

@nevermore3 nevermore3 added the wip Solution: work in progress label Sep 1, 2021
@nevermore3 nevermore3 force-pushed the format_fetchedges branch 5 times, most recently from 517b838 to 1b95198 Compare September 2, 2021 02:58
@nevermore3
Copy link
Contributor Author

subjob of #2594

@nevermore3 nevermore3 added the doc affected PR: improvements or additions to documentation label Sep 24, 2021
@nevermore3 nevermore3 force-pushed the format_fetchedges branch 4 times, most recently from ef94fd8 to 7f5a074 Compare September 24, 2021 11:14
@nevermore3 nevermore3 added ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Sep 24, 2021
@nevermore3 nevermore3 force-pushed the format_fetchedges branch 3 times, most recently from c360665 to e2d5779 Compare September 26, 2021 03:19
@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2021

Codecov Report

Merging #2779 (619005b) into master (dddf54e) will decrease coverage by 0.99%.
The diff coverage is 47.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2779      +/-   ##
==========================================
- Coverage   85.47%   84.48%   -1.00%     
==========================================
  Files        1229     1252      +23     
  Lines      110375   111705    +1330     
==========================================
+ Hits        94341    94370      +29     
- Misses      16034    17335    +1301     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.h 92.30% <ø> (ø)
src/clients/storage/GraphStorageClient.h 100.00% <ø> (ø)
src/clients/storage/InternalStorageClient.cpp 0.00% <0.00%> (ø)
src/clients/storage/InternalStorageClient.h 0.00% <ø> (-100.00%) ⬇️
src/clients/storage/StorageClientBase.h 59.32% <ø> (+6.94%) ⬆️
src/common/utils/NebulaKeyUtils.cpp 89.03% <0.00%> (-4.89%) ⬇️
src/common/utils/NebulaKeyUtils.h 80.68% <ø> (+2.65%) ⬆️
src/graph/executor/mutate/InsertExecutor.cpp 100.00% <ø> (ø)
src/graph/planner/plan/Admin.cpp 54.23% <0.00%> (-2.40%) ⬇️
src/graph/planner/plan/PlanNode.h 80.00% <ø> (ø)
... and 215 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 7239e5f...619005b. Read the comment docs.

@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 27, 2021
* attached with Common Clause Condition 1.0, found in the LICENSES directory.
*/

#ifndef GRAPH_PLANNER_NGQL_FETCH_EDGES_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

pragma once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, the effect is the same

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the new code style according to the doc.

@@ -1,4 +1,4 @@
/* Copyright (c) 2020 vesoft inc. All rights reserved.
/* Copyright (c) 2021 vesoft inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't modify this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore it

@@ -21,10 +21,10 @@ class FetchEdgesValidatorTest : public ValidatorTestBase {
};

TEST_F(FetchEdgesValidatorTest, FetchEdgesProp) {
auto src = VariablePropertyExpression::make(pool_.get(), "_VAR1_", kSrc);
auto src = ColumnExpression::make(pool_.get(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to ColumnExpression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the column names of the first three columns in the dataset are fixed, we can use columnExpresion instead of VariableProperyExpression

Copy link
Contributor

@Shylock-Hg Shylock-Hg left a comment

Choose a reason for hiding this comment

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

LGTM, but don't modify the unrelated behavior

@CPWstatic CPWstatic merged commit 722d5c5 into vesoft-inc:master Sep 28, 2021
@nevermore3 nevermore3 deleted the format_fetchedges branch September 28, 2021 08:44
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Sep 14, 2023
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-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants