Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

Add export/import util - #100

Merged
jmatak merged 32 commits into
mainfrom
add-export-util
Apr 6, 2022
Merged

jmatak merged 32 commits into
mainfrom
add-export-util

Conversation

@katarinasupe

@katarinasupe katarinasupe commented Dec 2, 2021 •

Copy link
Copy Markdown
Contributor

Description

Addition of import and export util. It adds the ability to import and export graphs in a specific formats.

Pull request type

  • Algorithm/Module

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

  • Core algorithm/module implementation
  • Query module implementation
  • Unit tests
  • End-to-end tests
  • Code documentation
  • README short description
  • Documentation on memgraph/docs

######################################

@katarinasupe

Copy link
Copy Markdown
Contributor Author

I have a few comments/concerns:

  1. Theoretically speaking, I supposed it is enough to count out_edges of every vertex to get all edges? When I thought about it, I concluded that if a vertex has in_edge, then that edge is also an out_edge of another vertex.
  2. I noticed an issue when exporting to a local JSON file. I need to change permissions locally in order for this to work. Not sure if that could be handled better.
  3. I still didn't write any tests since I'm not sure how to write them since the return type is None (an empty mgp.Record).
  4. I know putting everything in the dictionary might seem silly, but mgp.Vertex.properties is not JSON serializable, as well as some other types, so I had to go this way until there is some kind of toJSON() method in mgp.

@katarinasupe katarinasupe added lang: python Issue on Python codebase status: ready PR is ready for review type: utility Something that everyone should use labels Dec 2, 2021
@jmatak

jmatak commented Dec 6, 2021

Copy link
Copy Markdown
Contributor
  1. I think this is absolutely alright thing to do
  2. Permissions and how do they work with memgraph user is something that @antonio2368 is more familiar with. It definitely has to do something with it
  3. You won't be able to write e2e tests, but a working unit test (checking only the functionality of methods in a module) would work just fine. But it would require moving the functions to the python/mage directory
  4. This would be an interesting suggestion for mgp module. I don't think it should contain JSON, but a transformation to Python Dict would be helpful

@katarinasupe

Copy link
Copy Markdown
Contributor Author
  1. Yaay!
  2. Okay, I will ask Antonio :)
  3. I'll try writing the unit test and will let you know when it's done
  4. Will make an issue on memgraph repository and see how it goes

@katarinasupe

Copy link
Copy Markdown
Contributor Author

Asked Antonio about permissions, and the possible solution is:
Create folder "folder_name" which will be the folder for exported files. With
sudo chown memgraph "folder_name"
give permission to the user memgraph, since that user has started the process.
After that, memgraph will be able to write to the files within that folder.
I tried that and it works, it just has to be described correctly in the documentation.

@katarinasupe katarinasupe added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Jan 11, 2022
@katarinasupe katarinasupe mentioned this pull request Jan 27, 2022
@katarinasupe katarinasupe added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Jan 27, 2022
@katarinasupe

Copy link
Copy Markdown
Contributor Author

I added test here, and I don't see that I should do any error handling here, since I am only communicating with memgraph and writing to file. Everything necessary for the output file is defined in the docs, mentioned above.

Comment thread python/export_util.py Outdated
Comment thread python/export_util.py Outdated
Comment thread python/import_util.py Outdated
Comment thread python/import_util.py Outdated
@katarinasupe

Copy link
Copy Markdown
Contributor Author

Thank you @g-despot, totally forgot about that!

@jmatak jmatak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Address these few small small comments, and add Typing to the functions where it is missing, and Voila, done

Comment thread README.md Outdated
Comment thread e2e/export_import_deprecated/test_online_export_import/input.cyp
Comment thread e2e/export_import_deprecated/test_online_export_import/test.yml Outdated
Comment thread python/import_util.py Outdated

@antepusic antepusic left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Solid (and really useful) work! I've only left a few short comments – address them and that's it 🙂

Comment thread python/export_util.py Outdated
Comment thread python/mage/export_import_util/parameters.py
Comment thread python/export_util.py
Comment thread python/export_util.py Outdated
Comment thread python/import_util.py Outdated
Comment thread python/import_util.py
Comment thread python/import_util.py Outdated
@antoniofilipovic antoniofilipovic added status: change PR reviewed - needs changes and removed status: ready PR is ready for review labels Mar 21, 2022
Comment thread python/export_util.py
Comment thread python/mage/export_import_util/parameters.py
@katarinasupe katarinasupe added status: ready PR is ready for review and removed status: change PR reviewed - needs changes labels Mar 23, 2022
@jmatak
jmatak merged commit ef85d12 into main Apr 6, 2022
@jmatak
jmatak deleted the add-export-util branch April 6, 2022 13:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lang: python Issue on Python codebase status: ready PR is ready for review type: utility Something that everyone should use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants