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

Make use of sub-queries, to get rid of most apoc calls. #222

Merged
merged 6 commits into from
Nov 11, 2021

Conversation

Andy2003
Copy link
Collaborator

@Andy2003 Andy2003 commented May 7, 2021

resolves #214

@Andy2003 Andy2003 added the enhancement New feature or request label May 7, 2021
@Andy2003 Andy2003 requested a review from jexp May 7, 2021 17:49
@github-actions
Copy link

github-actions bot commented May 7, 2021

Unit Test Results

    5 files  ±0      5 suites  ±0   1m 39s ⏱️ +35s
186 tests ±0  182 ✔️ ±0  4 💤 ±0  0 ±0 
718 runs  +1  712 ✔️ +1  6 💤 ±0  0 ±0 

Results for commit 710a9a2. ± Comparison against base commit cbdacfe.

♻️ This comment has been updated with latest results.

@Andy2003
Copy link
Collaborator Author

Andy2003 commented May 7, 2021

only thing left is updating to ne next release of the neo4j-cypher-dsl

@Andy2003 Andy2003 force-pushed the feature/gh-214-subqueries branch from 7e59840 to e3422ff Compare May 12, 2021 18:22
@Andy2003 Andy2003 marked this pull request as ready for review May 18, 2021 14:50
@Andy2003 Andy2003 force-pushed the feature/gh-214-subqueries branch from e3422ff to a5c24f5 Compare May 20, 2021 07:50
@michael-simons
Copy link

only thing left is updating to ne next release of the neo4j-cypher-dsl

Feel free to ping me if needed.
Next week after hack/commit/push is totally fine.

}) YIELD value
WITH value[head(keys(value))] AS createPerson
CALL {
WITH $createPersonName AS name
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to watch out here for naming conflicts if we rename to a single/simple name, we should still be able to use parameters in the subqueries

Copy link
Collaborator Author

@Andy2003 Andy2003 Jun 1, 2021

Choose a reason for hiding this comment

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

I see no potential conflict here:

  • $createPersonName is defined by the library
  • name is the name of the fields' argument defined by the user

@Andy2003 Andy2003 requested a review from jexp June 1, 2021 18:53
@@ -14,7 +14,7 @@ typealias CypherDSL = org.neo4j.cypherdsl.core.Cypher

enum class FieldOperator(
val suffix: String,
val op: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this not a val anymore? shouldn't enums only have immutable fields?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a field anymore it is just used inside the constructor and so used as parameter only.

# Conflicts:
#	core/pom.xml
#	core/src/main/kotlin/org/neo4j/graphql/Predicates.kt
#	core/src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt
@@ -602,7 +602,10 @@ It allows you, without code to *compute field values* using complex queries.

You can also write your own, *custom top-level queries and mutations* using Cypher.

Arguments on the field are passed to the Cypher statement as parameters.
Arguments on the field are passed to the Cypher statement and can be used by name. They must not be prefixed by `$` since they are no longer parameters. Just use the same name as the fields' argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

but then this means it's a breaking change and users will have to rewrite their schemas before they can use this version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We transform old queries with the regex you found earlier and give some warning, so we should be compatible

.referralDate,
referredBy: userReferredByReferredBy {
.name
}
}][0],
referred: [(user)<-[userReferred:REFERRED_BY]-(userReferredUser:User) | userReferred {
} AS userReferredBy LIMIT 1
Copy link
Contributor

Choose a reason for hiding this comment

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

note that if any of those subqueries doesn't return a result, it will cause teh whole query to not emit any results
that's a big difference to how pattern comprehsensions work because they are independent and would just create an empty list instead

probably better WITH ... LIMIT 1 RETURN collect(userReferredByReferredBy) which would then at least return an empty list instead (or one element list)

Copy link
Contributor

Choose a reason for hiding this comment

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

OR you need to use OPTIONAL MATCH here WHICH RETURNs null rows for each input row if not-matching.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will otherwise lead to critical bugs

Copy link
Contributor

@jexp jexp left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much. Sorry it took so long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try to replace apoc calls by subqueries
3 participants