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

disable fetch & go missing yield clause #3056

Merged
merged 10 commits into from
Oct 25, 2021
Merged

disable fetch & go missing yield clause #3056

merged 10 commits into from
Oct 25, 2021

Conversation

nevermore3
Copy link
Contributor

@nevermore3 nevermore3 commented Oct 13, 2021 •
edited
Loading

1、 Must add YIELD clause in the FETCH vertex、FETCH edge & GO sentence
2、Remove the default output columns 【src, dst, rank】in fetch edge
3、Remove the default output column 【VertexID】in fetch vertex

@nevermore3 nevermore3 added the wip Solution: work in progress label Oct 13, 2021
@Sophie-Xie Sophie-Xie added this to the v2.7.0 milestone Oct 14, 2021
@nevermore3 nevermore3 force-pushed the yield_fetch branch 8 times, most recently from 3e4407a to 0dc3626 Compare October 14, 2021 11:23
@nevermore3 nevermore3 added doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Oct 14, 2021
@Sophie-Xie Sophie-Xie modified the milestones: v2.7.0, v3.0.0 Oct 15, 2021
@nevermore3
Copy link
Contributor Author

subjob of #3135

@nevermore3 nevermore3 added the incompatible PR: incompatible with the recently released version label Oct 20, 2021
@nevermore3 nevermore3 changed the title disable missing yield clause disable fetch & go missing yield clause Oct 20, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021 •
edited
Loading

Codecov Report

Merging #3056 (3a8bffc) into master (50b1760) will increase coverage by 0.05%.
The diff coverage is 86.99%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3056      +/-   ##
==========================================
+ Coverage   85.17%   85.23%   +0.05%     
==========================================
  Files        1294     1295       +1     
  Lines      118261   118191      -70     
==========================================
+ Hits       100733   100740       +7     
+ Misses      17528    17451      -77
Impacted Files Coverage Δ
src/common/datatypes/Geography.h 56.75% <ø> (ø)
src/common/time/ScopedTimer.h 100.00% <ø> (ø)
src/common/time/test/ScopedTimerTest.cpp 100.00% <ø> (ø)
src/common/utils/IndexKeyUtils.h 89.68% <ø> (ø)
src/graph/executor/Executor.cpp 84.02% <ø> (ø)
src/graph/executor/Executor.h 100.00% <ø> (ø)
src/graph/executor/admin/CharsetExecutor.cpp 100.00% <ø> (ø)
src/graph/executor/admin/ConfigExecutor.cpp 94.54% <ø> (ø)
src/graph/executor/admin/GroupExecutor.cpp 67.90% <ø> (ø)
src/graph/executor/admin/PartExecutor.cpp 96.55% <ø> (ø)
... and 205 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 cd2ecb9...3a8bffc. Read the comment docs.

@Shylock-Hg
Copy link
Contributor

Shylock-Hg commented Oct 21, 2021 •
edited
Loading

Good job! Agree with removing the default return column. But shall we give default result without YIELD clause? And YIELD just select the properties of the default result.

@nevermore3
Copy link
Contributor Author

Good job! Agree with removing the default return column. But shall we give default result without YIELD clause? And YIELD just select the properties of the default result.

According to the offline discussion, must add the yield clause

yixinglu
yixinglu approved these changes Oct 22, 2021
View reviewed changes
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

@HarrisChu
Copy link
Contributor

Agree too!
How about add some invalid cases, e.g. FETCH PROP ON person $a.VertexID, then the error would be raised.

@nevermore3
Copy link
Contributor Author

Agree too! How about add some invalid cases, e.g. FETCH PROP ON person $a.VertexID, then the error would be raised.

these cases already exists in unit tests

Shylock-Hg
Shylock-Hg approved these changes Oct 25, 2021
View reviewed changes
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

@yixinglu yixinglu merged commit 21a3ffb into vesoft-inc:master Oct 25, 2021
Shylock-Hg
Shylock-Hg reviewed Oct 25, 2021
View reviewed changes
tests/tck/features/go/GO.feature
@@ -306,7 +307,7 @@ Feature: Go Sentence
| EMPTY | "Russell Westbrook" |
When executing query:
"""
GO FROM "Russell Westbrook" OVER * REVERSELY
GO FROM "Russell Westbrook" OVER * REVERSELY YIELD like._dst, serve._dst, teammate._dst
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not user-friendly.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, why not disable over *?

Copy link
Contributor Author

@nevermore3 nevermore3 Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Alternative syntax: go from "A" over * YIELD dst(edge)

@jamieliu1023 jamieliu1023 mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czpmango czpmango czpmango left review comments

@Shylock-Hg Shylock-Hg Shylock-Hg approved these changes

@yixinglu yixinglu yixinglu approved these changes

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

Assignees
No one assigned
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
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nevermore3 @codecov-commenter @Shylock-Hg @HarrisChu @yixinglu @czpmango @Sophie-Xie