-
Notifications
You must be signed in to change notification settings - Fork 513
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
Neo4j parametrized from existing SCollection #4719
Neo4j parametrized from existing SCollection #4719
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4719 +/- ##
==========================================
- Coverage 60.90% 60.89% -0.01%
==========================================
Files 286 286
Lines 10484 10485 +1
Branches 761 761
==========================================
Hits 6385 6385
- Misses 4099 4100 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
scio-neo4j/src/main/scala/com/spotify/scio/neo4j/ops/Neo4jSCollectionReadOps.scala
Outdated
Show resolved
Hide resolved
scio-neo4j/src/main/scala/com/spotify/scio/neo4j/ops/Neo4jSCollectionReadOps.scala
Outdated
Show resolved
Hide resolved
Thanks a lot for the PR. I took the liberty of updating your branch directly to address some comments. |
.withSessionConfig(neo4jConf.sessionConfig) | ||
.withTransactionConfig(neo4jConf.transactionConfig) | ||
.withCypher(cypher) | ||
.withParametersFunction(neo4jInType.to(_).asMap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice! Yes, agree that this looks much better. 🙂
Just to confirm that my understanding is correct: it would be incumbent upon the user to write the Cypher query String such that the parameter names match the field names used by the (implicit) ValueType[T]
implementation? (Of course, in most cases, it would probably be easiest to use a case class (with the existing CaseMapper
-powered ValueType
) and match the query to the case class field names.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct.
The write API works the same way: if the cipher refers to a field which is not in the model (after the case mapper transform), there will be a runtime exception.
* Neo4j parallel-query from existing SCollection * Apply review comments * Simplify tests * revise scaladoc to be consistent with refactor * Remove useless interpolation --------- Co-authored-by: Michel Davit <micheld@spotify.com>
Expands the Scio Neo4j API to expose the ability to execute parameterized Cypher queries in parallel based upon inputs from an existing
SCollection
, as supported by the BeamNeo4jIO
implementation.In the original Scio-Neo4j PR (#4488), it appears that there was some discussion regarding a prospective general approach to sharding / parallel-reading. The implementation in this PR makes no such attempt to be transparent to the user, nor to calculate shard sizes automatically, but merely to expose the functionality already present in Beam. The client is responsible for:
SCollection
into reasonably-scoped querying shardsWHERE
clause parameters which limit query scope into the CypherString
SCollection
elements to values for those parameters