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

Enable scan in match. #5416

Merged
merged 15 commits into from
Mar 22, 2023
Merged

Enable scan in match. #5416

merged 15 commits into from
Mar 22, 2023

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Mar 20, 2023

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Close #5399 #5170

Description:

Remove limit requirement of scan in match, e.g. we can run match (v) return v directly now.

      """
      MATCH p = shortestPath( (a)-[e*..5]-(b) )
        WHERE id(a) == 'Tim Duncan' OR id(b) in ['Spurs', 'Tony Parker', 'Yao Ming']
        RETURN p
      """

This query failed in CI, but can't reproduce even in vesoft/nebula-dev:centos7 docker environment. And not caused by ScanVertices so skip it now. Could see detail of this in #5428

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

@Shylock-Hg Shylock-Hg added ready-for-testing PR: ready for the CI test doc affected PR: improvements or additions to documentation labels Mar 20, 2023
@Shylock-Hg Shylock-Hg marked this pull request as draft March 20, 2023 09:19
@Shylock-Hg Shylock-Hg marked this pull request as ready for review March 22, 2023 02:17
Copy link
Contributor

@codesigner codesigner left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -39,7 +39,7 @@ class JoinExecutor : public Executor {
size_t colSize_{0};
// If the join is natural join, rhsOutputColIdxs_ will be used to record the output column index
// of the right. If not, rhsOutputColIdxs_ will be empty.
std::vector<size_t> rhsOutputColIdxs_;
std::optional<std::vector<size_t>> rhsOutputColIdxs_;
Copy link
Contributor

Choose a reason for hiding this comment

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

why to change this field to optional? The original implementation seems OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When |a| join |a| will lead to error.

@Sophie-Xie Sophie-Xie merged commit 527c9b0 into vesoft-inc:master Mar 22, 2023
@Shylock-Hg Shylock-Hg deleted the feature/scan branch March 24, 2023 01:10
@wey-gu
Copy link
Contributor

wey-gu commented Mar 27, 2023

In documentation, we could remove the hacky query example as this issue mentioned also.

#5436

cc @cooper-lzy @abby-cyber

@abby-cyber
Copy link
Contributor

vesoft-inc/nebula-docs-cn#2705

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.

support full scan
7 participants