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

initial version of neo4j driver adapter #1924

Merged
merged 11 commits into from Apr 12, 2024
Merged

initial version of neo4j driver adapter #1924

merged 11 commits into from Apr 12, 2024

Conversation

ShaunakDas88
Copy link
Contributor

Apologies in advance for making this PR a single commit. This adds a Neo4J Java driver adapter to NoSQLBench. There are three op types added for running Cypher queries:

The full workload of resetting schema, creating schema, ingesting data, running ANN searches was run against a free-tier Neo4J Aura database, and all of the above appear to be working properly. One thing to keep in mind in the future is whether we are doing what is recommended for deletion of graph nodes during schema reset (a reference to support writeup is provided in the workload yaml)

In the case that we don't have the driver configured optimally, we can add in the future.

msmygit
msmygit previously approved these changes Apr 11, 2024
* @param unmasked The string to mask
* @return The masked string
*/
protected static String maskDigits(String unmasked) {
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit I think this would be a good candidate to be extracted into some common NB utility so that all drivers can utilize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. what place do we think if appropriate for this?

dave2wave
dave2wave previously approved these changes Apr 11, 2024
Copy link
Contributor

@dave2wave dave2wave left a comment

Choose a reason for hiding this comment

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

Interesting - I like that this is all one PR because it can be a good example of how to create a new driver.

for (int i = 0; i < n; i++) {
try {
idx = records[i].index(fieldName);
values[i] = records[i].get(idx).asObject();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If any one record does not have the required field, then the call to this method fails for all and exits the loop throwing an exception. Just verifying that this is the desired behavior as opposed to eg setting values[i] for that i to null or something and continuing iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, this was the intended behavior for this parsing of the Record array.

I figured if the user is leveraging this method, they are actually expecting the field from all Records that Neo4J returned to them. So if we throw an exception upon not getting the field, this forces the user to go back and check their data and/or data model.

Should we add a tryToGetFieldForAllRecords method here, that just populates an index in values with null,as you've suggested, if it's not found?

@jshook
Copy link
Contributor

jshook commented Apr 11, 2024

For the failing test, you can remove the offending maven dependency and test. This was a step towards symbolic verification of our floating point rounding error, but it will have to be added back in a more suitable way when we need it.

@ShaunakDas88
Copy link
Contributor Author

agreed, I saw Mark already removed it in his PR

@ShaunakDas88 ShaunakDas88 dismissed stale reviews from dave2wave and msmygit via c3422d2 April 12, 2024 04:04
@jshook
Copy link
Contributor

jshook commented Apr 12, 2024

I pushed up a set of changes to reconcile with (more recent changes to) main so it should merge cleanly. As long as it goes green, we'll merge it.

Copy link
Contributor

@jshook jshook left a comment

Choose a reason for hiding this comment

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

A nice addition!

@jshook jshook merged commit 2a24a9d into main Apr 12, 2024
5 checks passed
@jshook jshook deleted the adapter/neo4j branch April 12, 2024 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants