-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactored the types to match the most recent Apache Arrow version #39
base: master
Are you sure you want to change the base?
Conversation
Hey Mikko, thanks for the contribution! Looks like the PR failed to build in Travis: https://travis-ci.org/cldellow/sqlite-parquet-vtable/builds/611367321 I think that's expected, since the makefile is still linking against 0.9.0. I tried bumping it to 0.15.1 and building on a clean Ubuntu 16.04 c5n.2xlarge instance. But it failed: https://gist.github.com/cldellow/4634a192c1f08399975a2b730edee044 Looks like something to do with Before I spend some time digging into that, maybe you can save me some time :) Did you have to change something locally to get it to build? It's possible that if you were only building on OS X you didn't hit this issue - maybe your build was using your shared libraries. The Linux build currently builds static libraries so that the versioning story is straightforward for end users. |
I tweaked the Makefile to use 0.15.1 (in the upgrade-deps branch -- see diff). Now I get new build failures :) You can see them at https://gist.github.com/cldellow/ab127e57e2da4d48b2b517fa9e706296 When you have a moment, can you let me know if you have any ideas on what the issue might be? |
Hmm, that seems odd – maybe we need an explicit include for |
Yes – I'm actually building a dynamic library for OS X:s needs but I think the error you get on Ubuntu seems to be related to a missing header or something similar – not regards to linking? |
Arg, yes, I think you're right that's it to do with the headers. I had tweaked the includes list to update the location of some of the compression libraries, and that resulted in picking up a different If I fixed that, then I get:
This appears to be related to the long long changes - long term, we can do some platform specific ifdef, I guess. Although perhaps there's a third, more portable option that we should be using? For now, I just commented that change to make progress on getting it to build on Linux. At last, it built, but when trying to load the resulting shared library to run the tests, one gets:
which decodes to:
which makes me think my includes are still messed up, specifically the one providing the template definitions. This is probably a trivial bug for most people. :) But for me, I haven't programmed professionally in C++ for a long time, so I think I'll need to set aside at least a few hours to make progress. Hopefully in a week or so I'll have time available to make progress. |
See comment at #39 (comment) in #39 for log messages. I think I need to refactor the `-I` directives -- I think when I updated them to fix finding compression library headers, I brought in an internal set of arrow includes, perhaps?
Yes, this sounds good. Do you want me to give it a stab or do you have time to do it yourself?
I feel you – the last time I've done C++ professionally in the beginning of the millennium so a lot has changed since :D |
Re |
I added |
Hi!
Really love what you have done!
I managed to compile this now with OS X (that is still a bit messy so I'll have to refactor that a bit before maybe posting a PR about that) and also updated the code to work with the most recent Apache Arrow library.