perf: Neo4j csv publisher using apoc library for performance improvements - CLEAN #1877

zacr · 2022-05-27T21:24:53Z

Summary of Changes

New Neo4JCsvPublisher implementation using the APOC library for 5x performance improvement. This is a fresh PR because original one wasn't created on a clean branch.

Tests

I didn't see any automated tests for Neo4JCsvPublisher. This PR was tested manually by several community members.

Documentation

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does

VoLKyyyOG

Looks great! Once this is merged, I'll deploy into our dev environment and start testing other features out.

AFAIK this doesn't support multiple programmatic descriptions being added, so I'll work on that PR post-merge.

Excellent stuff!

chonyy · 2022-05-30T05:23:47Z

Looking forward to having this PR merged! Would like to use this in the official build 🚀

VoLKyyyOG · 2022-05-31T01:13:19Z

Ahh looks like DCO is blocked. Can you randomly add a small commit @zacr with format:

Signed-off-by: your_name_here <your_email_here>

(https://github.com/amundsen-io/amundsen/pull/1877/checks?check_run_id=6632089423)

Signed-off-by: Zac Ruiz <zac@salt.io>

chonyy · 2022-05-31T18:59:52Z

I tested this in my environment and found out that the data is actually not ingested into neo4j. Good news is after a discussion and pair debugging with @zacr , we are able to locate the problem! The root cause is related to the neo4j version.

Works with neo4j 3.5.26
Fails with neo4j 3.3.0

Basically, there are three main problems

Publisher not showing neo4j commit error

We will get this error message while running the query on the neo4j browser
org.neo4j.graphdb.TransactionFailureException: Transaction was marked as successful, but unable to commit transaction so rolled back.
However, this error is not showing on the publisher side. Therefore, it's hard to debug.

apoc.merge.node spec changed `3.3.0` and `3.5.26`

3.5.26: take 4 input parameters
3.3.0: take 3 input parameters

Property key case sensitive

This is the query written in the source code

CALL apoc.periodic.iterate('UNWIND $rows AS row RETURN row', '
                        CALL apoc.merge.node([row.label], {key:row.key}, row, {published_tag:$tag,publisher_last_updated_epoch_ms:timestamp()}) YIELD node RETURN COUNT(*);
                    ', {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }})

This is the header of the csv file

"LABEL","KEY","name","is_view"

In 3.5.26 row.key could be mapped to KEY. In 3.3.0, we think it's case sensitive and it's not able to find the uppercase "KEY" when written in lowercase in the query.

VoLKyyyOG · 2022-06-01T02:43:24Z

Ahh yep, that would make sense with the .lower() I saw in the previous iteration. Thanks for investigating this as I'm not too familiar with Neo4J!

zacr · 2022-06-09T13:24:39Z

Thanks so much @chonyy. I have an update in progress that addresses all three, hope to have it committed in the next day or so.

chonyy · 2022-06-11T12:01:29Z

Thanks so much @chonyy. I have an update in progress that addresses all three, hope to have it committed in the next day or so.

@zacr that's great news! I could also help with testing it in my environment once you are done 🚀

chonyy · 2022-06-12T14:16:05Z

Hey @zacr ,

FYI, I tested your code in neo4j 3.5.26 and apoc 3.5.0.17, it also doesn't work. The error message is like below.

After I changed all the key to uppercase, it works! Just want to let you know that case sensitivity seems to also be an issue in your provided environment.

zacr · 2022-06-13T13:54:15Z

Awesome @chonyy . Agreed on the upper case. Can we assume the headers of the CSV files will always be upper case?

zacr · 2022-06-13T13:55:56Z

Also, I am out of town this week, but here is the error handling code that goes right after session.run() in _execute_statement()

            ret = [dict(i) for i in result]

            if ret[0]['failedOperations'] > 0:
                raise RuntimeError(f"Failed to executed statement: {stmt} with {ret[0]['errorMessages']}")

Was thinking of adding a flag to turn on\off this checking.

ozandogrultan · 2022-08-11T19:43:08Z

Any updates on this PR? Would be great to see it merged!

chonyy · 2022-08-12T01:13:27Z

I'm thinking that maybe we don't even have to spend time supporting Neo4j 3.3.0 since it's already EOL. @zacr's version works great with some minor fixes related to case sensitivity.

What I can help here is to add the fix and also the extra part that zacr mentioned, in order to surface the error to the caller side. But we still need a commiter's help to look into this and approve the CI pipeline run 🥲

conker84

Hi I just checked your code and generally seems good to me, I have just a couple of nitpicks and one related to the index creation that it's a bug (so the performance improvement could be greater than your initial estimation).

One important thing that seems missing is the support for a database that is not the default one. Neo4j since version 4.X supports multi-tenancy so it would be good to have it in there.

conker84 · 2022-11-25T14:08:49Z

databuilder/databuilder/publisher/neo4j_csv_publisher_apoc.py

+        CALL apoc.periodic.iterate(
+            'UNWIND $rows AS row RETURN row', 
+            'CALL apoc.merge.node([row.label], {key:row.key}, row, row) YIELD node RETURN COUNT(*);', 
+            {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }}


Suggested change

{batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }}

{batchSize: $batch_size, parallel: False, params: { rows: $batch, tag: $publish_tag }}

I would like to remove iterateList as it's deprecated and use the default batchMode which is BATCH, that has the same behaviour.

conker84 · 2022-11-25T14:09:11Z

databuilder/databuilder/publisher/neo4j_csv_publisher_apoc.py

+            return """            
+                    CALL apoc.periodic.iterate('UNWIND $rows AS row RETURN row', '
+                        CALL apoc.merge.node([row.label], {key:row.key}, row, {published_tag:$tag,publisher_last_updated_epoch_ms:timestamp()}) YIELD node RETURN COUNT(*);
+                    ', {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }})


Suggested change

', {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }})

', {batchSize: $batch_size, parallel: False, params: { rows: $batch, tag: $publish_tag }})

like above

conker84 · 2022-11-25T14:09:43Z

databuilder/databuilder/publisher/neo4j_csv_publisher_apoc.py

+        return """            
+            CALL apoc.periodic.iterate('UNWIND $rows AS row RETURN row', '
+                CALL apoc.merge.node([row.label], {key:row.key}, row, row) YIELD node RETURN COUNT(*);
+            ', {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch }})


Suggested change

', {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch }})

', {batchSize: $batch_size, parallel: False, params: { rows: $batch }})

like above

conker84 · 2022-11-25T14:18:28Z

databuilder/databuilder/publisher/neo4j_csv_publisher_apoc.py

+        #    CALL apoc.schema.assert(null, data, dropExisting: false) YIELD label, key, keys, unique, action
+        # """
+        stmt = """
+            CREATE CONSTRAINT ON (node:label) ASSERT node.key IS UNIQUE


The label is fixed you cannot pass it as a parameter as you do here you have to format the string upfront otherwise it won't work

Golodhros · 2023-01-31T21:44:55Z

/rebase

kristenarmes · 2023-02-01T00:49:01Z

Hello, I know it has been a while since there has been activity on this PR, but I wanted to follow up for those who were interested in a faster publisher. A few months ago I worked on one myself in this PR using the unwind clause, which improves performance by allowing Neo4j to compile and cache the statements and reduces the overall amount of transactions (see it here). This publisher doesn't depend on the apoc library. We have been using it and seeing huge improvements in speed compared to the old one. I haven't tested it on all versions of Neo4j, but it may be sufficient for others to try and see if this in progress one using apoc is no longer required?

Golodhros · 2023-02-08T00:58:25Z

Closing as done.

zacr requested a review from a team as a code owner May 27, 2022 21:24

boring-cyborg bot added the area:databuilder From databuilder folder label May 27, 2022

zacr mentioned this pull request May 27, 2022

perf: Neo4j csv publisher using apoc library for performance improvements #1836

Closed

4 tasks

zacr changed the title ~~Fresh commit of apoc based neo4j csv publisher~~ perf: Neo4j csv publisher using apoc library for performance improvements - CLEAN May 27, 2022

VoLKyyyOG reviewed May 30, 2022

View reviewed changes

Zachary Ruiz and others added 5 commits May 31, 2022 09:53

Fresh commit of apoc based neo4j csv publisher

7017bec

Signed-off-by: Zac Ruiz <zac@salt.io>

Updated documentation

807a3f9

Signed-off-by: Zac Ruiz <zac@salt.io>

Updated documentation

9333263

Signed-off-by: Zac Ruiz <zac@salt.io>

Updated documentation

9c53870

Signed-off-by: Zac Ruiz <zac@salt.io>

Formatting. Signed-off-by: Zac Ruiz zac@salt.io

e62874d

Signed-off-by: Zac Ruiz <zac@salt.io>

zacr force-pushed the neo4j_csv_publisher_apoc_fresh branch from c51cb10 to e62874d Compare May 31, 2022 13:54

This was referenced Jun 14, 2022

feat and perf: Add support for Multiple Programmatic Descriptions and Table level Badges #1902

Closed

feat and perf: Add support for Multiple Programmatic Descriptions and Table level Badges #1903

Closed

feng-tao added the keep fresh Disables stalebot from closing an issue label Jun 18, 2022

chonyy mentioned this pull request Aug 12, 2022

Neo4jCsvPublisher Speed Optimization (Parallelism) #1610

Closed

conker84 reviewed Nov 25, 2022

View reviewed changes

Golodhros closed this Jan 31, 2023

Golodhros reopened this Jan 31, 2023

Golodhros closed this Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Neo4j csv publisher using apoc library for performance improvements - CLEAN #1877

perf: Neo4j csv publisher using apoc library for performance improvements - CLEAN #1877

zacr commented May 27, 2022

VoLKyyyOG left a comment

chonyy commented May 30, 2022

VoLKyyyOG commented May 31, 2022

chonyy commented May 31, 2022

VoLKyyyOG commented Jun 1, 2022

zacr commented Jun 9, 2022

chonyy commented Jun 11, 2022

chonyy commented Jun 12, 2022

zacr commented Jun 13, 2022

zacr commented Jun 13, 2022

ozandogrultan commented Aug 11, 2022

chonyy commented Aug 12, 2022 •

edited

Loading

conker84 left a comment

conker84 Nov 25, 2022

conker84 Nov 25, 2022

conker84 Nov 25, 2022

conker84 Nov 25, 2022

Golodhros commented Jan 31, 2023

kristenarmes commented Feb 1, 2023

Golodhros commented Feb 8, 2023

	{batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }}
	{batchSize: $batch_size, parallel: False, params: { rows: $batch, tag: $publish_tag }}

	', {batchSize: $batch_size, iterateList: True, parallel: False, params: { rows: $batch, tag: $publish_tag }})
	', {batchSize: $batch_size, parallel: False, params: { rows: $batch, tag: $publish_tag }})

perf: Neo4j csv publisher using apoc library for performance improvements - CLEAN #1877

perf: Neo4j csv publisher using apoc library for performance improvements - CLEAN #1877

Conversation

zacr commented May 27, 2022

Summary of Changes

Tests

Documentation

CheckList

VoLKyyyOG left a comment

Choose a reason for hiding this comment

chonyy commented May 30, 2022

VoLKyyyOG commented May 31, 2022

chonyy commented May 31, 2022

Publisher not showing neo4j commit error

apoc.merge.node spec changed 3.3.0 and 3.5.26

Property key case sensitive

VoLKyyyOG commented Jun 1, 2022

zacr commented Jun 9, 2022

chonyy commented Jun 11, 2022

chonyy commented Jun 12, 2022

zacr commented Jun 13, 2022

zacr commented Jun 13, 2022

ozandogrultan commented Aug 11, 2022

chonyy commented Aug 12, 2022 • edited Loading

conker84 left a comment

Choose a reason for hiding this comment

conker84 Nov 25, 2022

Choose a reason for hiding this comment

conker84 Nov 25, 2022

Choose a reason for hiding this comment

conker84 Nov 25, 2022

Choose a reason for hiding this comment

conker84 Nov 25, 2022

Choose a reason for hiding this comment

Golodhros commented Jan 31, 2023

kristenarmes commented Feb 1, 2023

Golodhros commented Feb 8, 2023

apoc.merge.node spec changed `3.3.0` and `3.5.26`

chonyy commented Aug 12, 2022 •

edited

Loading