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

Inconsistent LIMIT operations #3151

Closed
randomJoe211 opened this issue Oct 19, 2021 · 3 comments · Fixed by #3171
Closed

Inconsistent LIMIT operations #3151

randomJoe211 opened this issue Oct 19, 2021 · 3 comments · Fixed by #3171
Assignees
Labels
type/bug Type: something is unexpected
Milestone

Comments

@randomJoe211
Copy link
Contributor

randomJoe211 commented Oct 19, 2021

Nebula Graph version: 2021.10.18-nightly

LOOKUP ON player LIMIT 2 and LOOKUP ON player | LIMIT 2 return totally different numbers of rows.

  1. The newly added LIMIT clause in LOOKUP (without a pipe) is based on the partition level and the returned number of rows is not as specified in the LIMIT clause. This is inappropriate.
(root@nebula) [basketballplayer]> LOOKUP ON player LIMIT 2;
+-------------+
| VertexID    |
+-------------+
| "player100" |
| "player101" |
| "player102" |
| "player103" |
| "player104" |
| "player105" |
| "player106" |
| "player107" |
| "player108" |
| "player109" |
| "player112" |
| "player113" |
| "player114" |
| "player117" |
| "player122" |
| "player124" |
| "player127" |
| "player132" |
| "player150" |
+-------------+
Got 19 rows (time spent 1315/1901 us)

Tue, 19 Oct 2021 09:34:17 UTC

(root@nebula) [basketballplayer]> LOOKUP ON player | LIMIT 2;
+-------------+
| VertexID    |
+-------------+
| "player100" |
| "player101" |
+-------------+
Got 2 rows (time spent 1146/1713 us)

Tue, 19 Oct 2021 09:34:20 UTC
  1. Nebula Graph has the LIMIT syntax, the newly added LIMIT syntax conflicts with that of the old one. Now it is only added to LOOKUP, what if it is added to MATCH? Now MATCH required no pipe before LIMIT, if this new syntax is added, there will be a conflict.
(root@nebula) [basketballplayer]> MATCH (v:player) RETURN v | LIMIT 2;
+--------------------------------------------------------+
| v                                                      |
+--------------------------------------------------------+
| ("player105" :player{age: 31, name: "Danny Green"})    |
| ("player109" :player{age: 34, name: "Tiago Splitter"}) |
+--------------------------------------------------------+
Got 2 rows (time spent 2422/2940 us)

Tue, 19 Oct 2021 09:34:44 UTC

(root@nebula) [basketballplayer]> MATCH (v:player) RETURN v LIMIT 2;
+--------------------------------------------------------+
| v                                                      |
+--------------------------------------------------------+
| ("player105" :player{age: 31, name: "Danny Green"})    |
| ("player109" :player{age: 34, name: "Tiago Splitter"}) |
+--------------------------------------------------------+
Got 2 rows (time spent 2105/2599 us)

Tue, 19 Oct 2021 09:34:52 UTC

It is recommended to optimize LIMIT so that its behaviors can be reasonable, logical, and consistent throughout nGQL (openCypher-compatible syntax included).

@randomJoe211 randomJoe211 added the type/bug Type: something is unexpected label Oct 19, 2021
@bright-starry-sky
Copy link
Contributor

lookup is based on the partition level and returned rows number is partition_num * limit . this is the original design. It's not a bug.
In addition, the purpose of this design is related to the next index paging scan.
And match statement same with generic index scan , that's done in the Graph layer is to deal with the internal limits and the pipe limits.
doc need to be updated if necessary.
close this one.

@jievince
Copy link
Contributor

jievince commented Oct 19, 2021

It is recommended to optimize LIMIT so that its behaviors can be reasonable, logical, and consistent throughout nGQL (openCypher-compatible syntax included).

I totally agree with it!The syntax of the inconsistency will make the user difficult to understand.

@czpmango
Copy link
Contributor

Nebula Graph has the LIMIT syntax, the newly added LIMIT syntax conflicts with that of the old one. Now it is only added to LOOKUP, what if it is added to MATCH? Now MATCH required no pipe before LIMIT, if this new syntax is added, there will be a conflict.

I think that's a reasonable consideration.
We should be careful in grammatical design. Reopen this issue and really hope we can discuss this some more.

@czpmango czpmango reopened this Oct 20, 2021
@czpmango czpmango added the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Oct 20, 2021
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Oct 20, 2021
@Sophie-Xie Sophie-Xie removed the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Type: something is unexpected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants