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

fix go statement, id($$) filter is incorrect #4768

Merged
merged 2 commits into from
Oct 21, 2022
Merged

fix go statement, id($$) filter is incorrect #4768

merged 2 commits into from
Oct 21, 2022

Conversation

jievince
Copy link
Contributor

@jievince jievince commented Oct 21, 2022 •
edited
Loading

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

fix #4765

Description:

How do you solve it?

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

Because GetDstBySrc doesn't support filter push-down,
replacing GetNeighbors with GetDstBySrc in the following case will not bring performance benefits.
So this pr will not optimize this case.

GO FROM "Tony Parker" OVER like WHERE like._dst != "Tim Duncan" YIELD DISTINCT id($$)

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 .....

@jievince jievince added the ready-for-testing PR: ready for the CI test label Oct 21, 2022
@jievince jievince requested review from nevermore3, czpmango, Aiee, dutor and Shylock-Hg and removed request for nevermore3 October 21, 2022 04:51
Shylock-Hg
Shylock-Hg reviewed Oct 21, 2022
View reviewed changes
tests/tck/features/go/SimpleCase.feature
@@ -426,30 +331,6 @@ Feature: Simple case
| 2 | GetDstBySrc | 1 | |
| 1 | Start | | |
| 0 | Start | | |
When profiling query:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delete tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just move them

@Shylock-Hg
Copy link
Contributor

I think GetDstBySrc should support push-down filter too. But could add in future.

@jievince
Copy link
Contributor Author

I think GetDstBySrc should support push-down filter too. But could add in future.

Could add it in future. But I don't think it will gain much performance improvement against GetNeighbors

@Shylock-Hg
Copy link
Contributor

I think GetDstBySrc should support push-down filter too. But could add in future.

Could add it in future. But I don't think it will gain much performance improvement against GetNeighbors

Make sense.

@jievince jievince changed the title fix issue 4765 [DO NOT MERGE]fix issue 4765 Oct 21, 2022
@jievince jievince requested a review from Sophie-Xie October 21, 2022 08:04
@Sophie-Xie Sophie-Xie changed the title [DO NOT MERGE]fix issue 4765 fix issue 4765 Oct 21, 2022
@Sophie-Xie Sophie-Xie changed the title fix issue 4765 fix go statement, id($$) filter is incorrect Oct 21, 2022
@Sophie-Xie Sophie-Xie merged commit cea9c61 into vesoft-inc:master Oct 21, 2022
@jievince jievince deleted the fix-colnames-bug branch October 21, 2022 08:08
Sophie-Xie pushed a commit that referenced this pull request Oct 21, 2022
@jievince @Sophie-Xie
fix issue 4765 (#4768)
838085a
@Sophie-Xie Sophie-Xie added the cherry-pick-v3.3 PR: need cherry-pick to this version label Oct 21, 2022
Sophie-Xie added a commit that referenced this pull request Oct 21, 2022
@Sophie-Xie @HarrisChu
@SuperYoko @jievince @czpmango
Cherry pick 3.3 (1020-1021) (#4772)
e4d4cac 
* logging error if any error in step (#4759)

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

* fix storage job (#4762)

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

* fix issue 4765 (#4768)

* Avoid full scan of match when there is an Relational In predicate (#4748)

* Avoid match full scan when has in predicate

fix

small fix

add test case

small change

add test cases

small fix

fmt

* small delete

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

Co-authored-by: Harris.Chu <[email protected]>
Co-authored-by: Alex Xing <[email protected]>
Co-authored-by: jie.wang <[email protected]>
Co-authored-by: kyle.cao <[email protected]>
@wey-gu wey-gu mentioned this pull request Oct 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shylock-Hg Shylock-Hg Shylock-Hg left review comments

@czpmango czpmango czpmango approved these changes

@nevermore3 nevermore3 nevermore3 approved these changes

@Aiee Aiee Awaiting requested review from Aiee

@dutor dutor Awaiting requested review from dutor

@Sophie-Xie Sophie-Xie Awaiting requested review from Sophie-Xie

Assignees
No one assigned
Labels
cherry-pick-v3.3 PR: need cherry-pick to this version 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.

go statement, id($$) filter is incorrect
5 participants
@jievince @Shylock-Hg @nevermore3 @czpmango @Sophie-Xie