Skip to content
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

feat(rust): add the driver manager #1803

Merged
merged 31 commits into from
May 24, 2024
Merged

Conversation

alexandreyc
Copy link
Contributor

@alexandreyc alexandreyc commented May 1, 2024

Hey!

Here is the penultimate PR containing the driver manager for Rust.

The last PR will contain all the integration tests.

@github-actions github-actions bot added this to the ADBC Libraries 1.0.0 milestone May 1, 2024
@alexandreyc
Copy link
Contributor Author

CC @mbrobbel

@alexandreyc
Copy link
Contributor Author

CI is broken because we need the SQLite driver dynamic library in path to successfully run doctests... I have no idea how we can do that. Is there any similar situation elsewhere?

@lidavidm
Copy link
Member

lidavidm commented May 2, 2024

CI is broken because we need the SQLite driver dynamic library in path to successfully run doctests... I have no idea how we can do that. Is there any similar situation elsewhere?

You could consider DuckDB too? It supports ADBC in the DuckDB library itself so you can just download and point to it standalone without having to build anything else from this repo.

@alexandreyc
Copy link
Contributor Author

You could consider DuckDB too? It supports ADBC in the DuckDB library itself so you can just download and point to it standalone without having to build anything else from this repo.

Yes I could do that but I also have integration tests against SQLite and PostgreSQL drivers so I ideally would prefer to build those as part of the CI.

Related question: shouldn't we release compiled library for drivers too?

@lidavidm
Copy link
Member

lidavidm commented May 2, 2024

Release them where? That's always the problem :)

There is native-unix.yml which is set up to build drivers in one portion of the pipeline then share them across several builds.

@alexandreyc
Copy link
Contributor Author

Release them where? That's always the problem :)

To be honest, I haven't given the question too much thought 😃

There is native-unix.yml which is set up to build drivers in one portion of the pipeline then share them across several builds.

Nice, I'll look into it. Thanks!

@lidavidm lidavidm removed this from the ADBC Libraries 1.0.0 milestone May 3, 2024
@github-actions github-actions bot added this to the ADBC Libraries 1.0.0 milestone May 3, 2024
Copy link
Contributor

@mbrobbel mbrobbel left a comment

Choose a reason for hiding this comment

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

Looks good!

rust/core/src/lib.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Show resolved Hide resolved
rust/core/src/driver_manager.rs Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
@lidavidm lidavidm removed this from the ADBC Libraries 12 milestone May 7, 2024
rust/core/src/driver_manager.rs Outdated Show resolved Hide resolved
rust/core/src/driver_manager.rs Show resolved Hide resolved
@alexandreyc alexandreyc force-pushed the rust-part4 branch 4 times, most recently from 7ad9c26 to 5b0ff9a Compare May 8, 2024 15:47
@lidavidm lidavidm added this to the ADBC Libraries 13 milestone May 9, 2024
@lidavidm
Copy link
Member

lidavidm commented May 9, 2024

Sorry, this missed the last release...it'll be in the next one

@lidavidm
Copy link
Member

lidavidm commented May 9, 2024

@mbrobbel are we all good here?

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

CI LGTM, though, maybe we should think about refactoring things at some point (since not all the jobs in native-unix/native-windows need to run for Rust-only changes). Though, it's hard to do this without a unified build dependency graph, and at some point I assume Rust will feed back into the other builds...

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@alexandreyc
Copy link
Contributor Author

CI LGTM, though, maybe we should think about refactoring things at some point (since not all the jobs in native-unix/native-windows need to run for Rust-only changes). Though, it's hard to do this without a unified build dependency graph, and at some point I assume Rust will feed back into the other builds...

CI still needs some work, I haven't managed to make it work yet. I should be able to work on it in the next few days.

Yeah I agree with you that the current approach is a bit brute-force and will need refactoring at some point.

@alexandreyc alexandreyc force-pushed the rust-part4 branch 3 times, most recently from c6dee0b to 9bef652 Compare May 13, 2024 12:50
@alexandreyc
Copy link
Contributor Author

alexandreyc commented May 13, 2024

CI is now working on macOS and Ubuntu. Unfortunately I'm not at all familiar with Windows and would need some help to figure it out.

Basically, we need to set the equivalent of LD_LIBRARY_PATH (or put the DLL to a specific location?) for the dynamic linker to find DLLs built during native-windows workflow. See: https://github.com/apache/arrow-adbc/actions/runs/9063466338/job/24900436623

@lidavidm lidavidm merged commit 42a6e81 into apache:main May 24, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants