-
Notifications
You must be signed in to change notification settings - Fork 86
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
Cloud storage #1582
Cloud storage #1582
Conversation
7476aaa
to
1ae20a7
Compare
Emmm, my changes require system installed curl. I see your test is on self-host machines. Is it possible to install CURL on that. |
Hi @yuchenZhangTG we would like to ensure that Kùzu can be built without installing any third party dependencies manually. If your change requires |
As mentioned in https://arrow.apache.org/docs/dev/developers/cpp/building.html,
System CURL is required by AWSSDK and google_cloud_cpp_storage. It seems custom build CURL cannot be detected in the dependency build of Apache arrow. The dependency build generate its own cmake commands and environment variables. I tried to pass in different -D and environemnt variables, but it does not work. Also I tried to add CURL in the external folder next to Apache arrow. Since third_party is built after apache arrow. @mewim How do you think. |
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.
Hi @yuchenZhangTG , this feature looks awesome! We do hope to integrate with cloud storage well and have some ideas in our roadmap (though not publicly written down yet).
As for this PR, can you suggest a good way for testing? Should we have a private bucket for some of our testing dataset on S3/GCS, and deploy secrets on our CI machine?
Btw, feel free to join our slack workspace here (https://join.slack.com/t/kuzudb/shared_invite/zt-1w0thj6s7-0bLaU8Sb~4fDMKJ~oejG_g) :smile We'd like to learn more from your use cases with this feature!
@ray6080 For testing, yeah, it would be great if you set up your own AWS and GCP account, and have private buckets for testing. The cost for cloud storage is not expensive and you should be able to cover the cost. It is better to have secrets hosted on your CI test machines (Sometimes, people put secrets in the Github code, it is convenient, but it is not safe). As far as I know, GCP does not charge for downloading, and AWS only charge for cross-region download. It is also OK to make buckets public if you want to share the use case on the Kuzu cloud (the one on your website demo). For cloud import |
Thanks! @yuchenZhangTG . This looks promising. We will set up a testing pipeline for this asap after our coming release next week. |
The option in DuckDB is HTTPFS, so it is HTTP (support http) + FS (support AWS S3). My feature does not support HTTP but S3 and GCS. So it is better call it DuckDB write their own HTTPFS extension. The extension is separatable from their package, and user install it using Another solution For pre-compiled package, I suggested the build with filesystem. How do you think ? @ray6080 |
You're right. How about we be more explicit and call this
Yep. I agree that it's not a good idea to do the fs extension with arrow. We have a battle plan to remove arrow inside the team, that's why i mentioned the extension idea.
This sounds good to me. @mewim what do you think?
Agreed! |
@ray6080 @yuchenZhangTG This sounds good to me. I double checked and it seems that we have installed
|
@mewim
Build not work because libcurl4-openssl-dev is missing. |
cd04183
to
eca9c5a
Compare
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.
Thanks @yuchenZhangTG ! The PR looks good to me. I hope to take another quick look before we merge.
Btw, can you take a look at our CLA file? And please add "I have read and agree to the terms under CLA.md." to the PR description if you're fine with it. Thanks!
I have redeployed all the CI runners with |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1582 +/- ##
==========================================
- Coverage 91.19% 91.10% -0.10%
==========================================
Files 779 779
Lines 28606 28654 +48
==========================================
+ Hits 26088 26104 +16
- Misses 2518 2550 +32
☔ View full report in Codecov by Sentry. |
Thanks @yuchenZhangTG . Can you manually squash your commits into one? That's a convention we're following. Then I think it's ready to merge! |
846d8e9
to
cfed035
Compare
Sounds great. |
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.
@yuchenZhangTG It seems there is something wrong with the asan test run, which lasts unusually long. Better let's figure out what happens before merging.
Meanwhile, I've set up a public GCS bucket here (https://storage.googleapis.com/kuzu-test/tinysnb) with csv files under here. Can you verify if this is good for use and help us to set up a test case on this?
My idea is we create a new dataset directory tinysnb-cloud
under our dataset directory with the schema.cypher
and copy.cypher
file, but replace COPY statements in copy.cypher
. Then I will add a new test file under our copy tests, which will automatically trigger copying the dataset on cloud into our database and run a bunch of queries.
Does this sound good to you?
@ray6080 I added a GCS test case in the end2end copy test. It seems that GCS anonymous access need to add a username |
fd67d73
to
001d7aa
Compare
Google cloud cpp is at least getting to the build stage, but it's failing for some reason still (it was working for me locally). I also still need to finish making the test be run only when the feature is enabled. On windows, the only way it seems that I could get it to detect curl in all of the kuzu build, the arrow build, and the google-cloud-cpp build, was defining |
86c82be
to
865e3e2
Compare
This also seems to be cause the arrow build to approach the maximum path length on windows. I suspect that the rust remote-fs feature will be difficult to build on windows as a result. But shortening the build directory paths a little helped, and it's succeeding now with the exception of coverage. Edit: actually it looks like there are a bunch of asan tests which are failing here but not on the master branch |
7a22537
to
c9f5e21
Compare
@yuchenZhangTG @benjaminwinger I'm proposing to close this PR as this has been a while, and we've now shifted to a new csv reader, instead of arrow. So this PR is not aligned with it anymore. Happy to re-open it if any of you are interested in continuing on this with the new csv reader, though a new series of PR for that sounds more reasonable. |
Close this for now as it is outdated for a while. |
copy Comment from "gs://kuzubi/example.csv";
copy Comment from "gs://kuzubi/example.csv.gz";
I have read and agree to the terms under CLA.md.