-
Notifications
You must be signed in to change notification settings - Fork 55
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
Parquet loader #1666
Parquet loader #1666
Conversation
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.
- Some minor cleanup
- Should support path being a folder of parquet files since that is what you get from Spark
# Conflicts: # raphtory/src/core/utils/errors.rs # raphtory/src/io/arrow/mod.rs # raphtory/src/python/graph/graph_with_deletions.rs
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.
Some small changes/suggestions, but otherwise looking good.
There are a couple of comments that I dont think need to go in this PR, but we probably want to get them tidied before the next major release.
Also for the testing it probably makes sense to load the graphs via pandas, parquet, raw, etc and see if they all create the same graph - can be part of a test refactoring ticket.
# Conflicts: # examples/rust/src/bin/btc/main.rs # examples/rust/src/bin/lotr/main.rs # raphtory/src/io/csv_loader.rs
What changes were proposed in this pull request?
Impl #1660
Why are the changes needed?
Same as above
Does this PR introduce any user-facing change? If yes is this documented?
Yes
How was this patch tested?
Unit tests
Issues
If this resolves any issues, please link to them here, the format is a KEYWORD followed by @_
KEYWORDS available are
close
,closes
,closed
,fix
,fixes
,fixed
,resolve
,resolves
,resolved
.Please delete this text before creating your PR
Are there any further changes required?
No