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

Standalone Order by, Skip/Offset, and Limit #1027

Merged

Conversation

JoelBergstrand
Copy link
Contributor

@JoelBergstrand JoelBergstrand commented Aug 19, 2024

Adds documentation for standalone Order by, Skip/Offset, and Limit clause and introduction of the Offset keyword. Implementation of CIP-168

Questions:

  • Should the ORDER BY, LIMIT, SKIP, OFFSET pages be collected into one page, or an additional page added, to reflect that they now can be used as one clause now?
  • Should the Clauses index page be modified to reflect that ORDER BY, LIMIT, SKIP, OFFSET can be used as a standalone clause instead of only as subclauses?

Corresponding PR

@JoelBergstrand JoelBergstrand marked this pull request as ready for review August 19, 2024 13:12
Copy link
Contributor

@gem-neo4j gem-neo4j left a comment

Choose a reason for hiding this comment

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

Great! I think we can discuss with Jens a little more on your questions :)

Comment on lines +158 to +162
MATCH (n)
ORDER BY n.name DESC
SKIP 2
LIMIT 2
RETURN collect(n.name) AS names
Copy link
Contributor

Choose a reason for hiding this comment

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

This query isn't really using it standalone though right? Would standalone not look more like: MATCH (n) LIMIT 2 RETURN collect(n.name) AS names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there could be a better way to word it, or possibly have to examples. One with only limit "standalone" and one example of them together as in the vast majority of cases it doesn't make sense to use it by itself without at least and order by.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, one could argue that a separate page or section would be more useful to show the existence of Order by n.name SKIP 1 LIMIT 1 as a standalone clause

Comment on lines +165 to +179
.Standalone use of `LIMIT`
[source, cypher]
----
MATCH (n)
LIMIT 2
RETURN collect(n.name) AS names
----

.Result
[role="queryresult",options="header,footer",cols="1*<m"]
|===
| names
| ["Andy", "Bernard"]
|Rows: 1
|===
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JPryce-Aklundh Here I think we should clarify that the ordering is not guaranteed, so users do not believe they will get ascending ordering by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We seem to have this disclaimer for SKIP which I believe is applicable here as well.

Neo4j does not guarantee the results generated by SKIP/OFFSET. The only clause that guarantees a specific row order is ORDER BY.

[[offset-synonym]]
== `OFFSET` as a synonym to `SKIP`

`OFFSET` was introduced as part of Cypher's xref:appendix/gql-conformance/index.adoc[] and can be used as a synonym to `SKIP`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is much better!

@JoelBergstrand
Copy link
Contributor Author

Great cleanup with the new images and examples!

Copy link
Contributor

@gem-neo4j gem-neo4j left a comment

Choose a reason for hiding this comment

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

This looks good to me :D

@neo-technology-commit-status-publisher
Copy link
Collaborator

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

@JPryce-Aklundh JPryce-Aklundh merged commit 8e0598a into neo4j:dev Sep 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants