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

refactor(c/driver/sqlite): port to driver base #1603

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

lidavidm
Copy link
Member

@lidavidm lidavidm commented Mar 8, 2024

Fixes #1141.
Fixes #1355.
Fixes #1602.

@lidavidm
Copy link
Member Author

lidavidm commented Mar 14, 2024

TODOs

  • Fix various TODOs in the sqlite base
  • (maybe for later) set up things such that error messages always get prefixed properly (probably: move export to Driver, and inject the prefix last second)
  • Figure out test timeouts (may have to come up with something...)
  • Merge base.h/driver.h
  • Add docstrings to driver
  • Test and fix Python
  • Fix error messages to fit the old format
  • Fix driver manager tests
  • Vendor fmt instead of using ExternalProject (appears we can't rely on ExternalProject in packaging builds)
  • Vendor a std::expected backport instead of relying on a crappy Result implementation (decided against this, there don't seem to be maintained backports)
  • Update R, other packages that list files to vendor with new paths
  • Windows
  • R on Windows
  • Almalinux/Debian/Ubuntu

@lidavidm lidavidm force-pushed the sqlite-driverbase branch 7 times, most recently from 2dcfc83 to 64a2075 Compare March 14, 2024 21:24
@lidavidm lidavidm force-pushed the sqlite-driverbase branch from 5b43bc3 to b03397f Compare March 15, 2024 21:26
@lidavidm
Copy link
Member Author

I'm still squashing a few CI issues and need to fill in some docs/comments/TODOs but overall this is ready.

@lidavidm lidavidm marked this pull request as ready for review March 15, 2024 21:29
@lidavidm lidavidm force-pushed the sqlite-driverbase branch from b03397f to 1225eb2 Compare March 15, 2024 21:53
@lidavidm
Copy link
Member Author

Hmm, clang doesn't like the #pragma and gcc 12.2 seems to have an issue with optional<vector>, time to jiggle the code until it works...

@lidavidm lidavidm force-pushed the sqlite-driverbase branch 3 times, most recently from f1ecfc0 to e1d27c7 Compare March 15, 2024 23:07
@lidavidm
Copy link
Member Author

@paleolimbot do you have any idea what's going on with Windows?

gcc -shared -s -static-libgcc -o adbcsqlite.dll tmp.def init.o c/driver/common/utils.o c/driver/framework/catalog.o c/driver/framework/objects.o c/driver/sqlite/sqlite.o c/driver/sqlite/statement_reader.o c/vendor/nanoarrow/nanoarrow.o c/vendor/sqlite3/sqlite3.o -lsqlite3 -LC:/rtools43/x86_64-w64-mingw32.static.posix/lib/x64 -LC:/rtools43/x86_64-w64-mingw32.static.posix/lib -LC:/R/bin/x64 -lR
C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: c/driver/framework/catalog.o:catalog.cc:(.text+0x69): undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long long&, unsigned long long)'
C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: c/driver/framework/catalog.o:catalog.cc:(.text+0x67a): undefined reference to `operator new(unsigned long long)'
C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: c/driver/framework/catalog.o:catalog.cc:(.text+0x6c1): undefined reference to `operator delete(void*, unsigned long long)'
C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: c/driver/framework/catalog.o:catalog.cc:(.text+0x818): undefined reference to `operator new(unsigned long long)'
...

The only thing I can think of is that maybe it should be linking with g++ and not gcc but at least on Linux that doesn't seem to affect things

@paleolimbot
Copy link
Member

I can do a checkout of this and see what's going on on Monday...it definitely should be linking with g++ but usually that's automatic if there's at least one .cc file in the src directory. Maybe changing init.c to int.cc (might require some extern "C"s) would do it?

@lidavidm
Copy link
Member Author

Thanks, I'll give that a shot in a bit

@lidavidm lidavidm force-pushed the sqlite-driverbase branch from b630a50 to 8747450 Compare March 16, 2024 16:31
@lidavidm
Copy link
Member Author

Thanks, that did the trick!

I guess that was a side effect of me re-arranging bootstrap.R so that I didn't have to adjust all the file lists by hand...

@lidavidm lidavidm force-pushed the sqlite-driverbase branch from 16a1adf to f56c27a Compare March 16, 2024 20:09
@lidavidm lidavidm force-pushed the sqlite-driverbase branch from ef320a7 to c74a5e0 Compare March 19, 2024 17:28
@lidavidm
Copy link
Member Author

I'd like to get this in for 0.11.0, if possible; and ideally also port PostgreSQL to this. Otherwise, I'll kick the PostgreSQL port, and further work on that driver, into the next release (#81, #855, #933, #1550 are what I have queued up).

@paleolimbot
Copy link
Member

I took a scan through these changes...there are a lot of them! The R changes look good...I can/will make them more resilient to changes in the C++ dependency (I promise that it is still easier than trying to locate CMake on everybody's Linux distribution). A few questions about the C++ changes:

  • Is the intention to add tests for the driver framework at some point?
  • I appreciate that the advanced templating and metaprogramming allows for more compact syntax for those who know how to use it; however, I wonder if we are excluding any potential contributors by invoking some of these concepts. I am almost certainly just projecting my own feelings about this (e.g., I feel unqualified to review this but want to help).
  • Do we need another error handling pattern? There is already nanoarrow + AdbcError + whatever C-based thing the driver is using. I can see how it would be helpful in a client helper library and if it's truly needed that's OK...it mostly just read to me as out of place.

@lidavidm
Copy link
Member Author

  • Tests at some point, yes (there's a TODO); but I want to make sure the structure is acceptable
  • CRTP is a fairly common template pattern. Virtual calls probably aren't a big deal, though.
  • I could see the argument to stick with AdbcError but then I question what Error in the original framework was for

@paleolimbot
Copy link
Member

I question what Error in the original framework was for

I think it was just that something had to be the private_data of the AdbcError to handle the extra key/value metadata, and that also became the thing that you use to populate the AdbcError (for better or worse).

@lidavidm
Copy link
Member Author

Hmm, I'm willing to port back to AdbcError (I will refactor SetError/provide a different overload of that though, since it needs to pull the driver name from somewhere)

@lidavidm
Copy link
Member Author

Also cc @zeroshade for opinions

@lidavidm
Copy link
Member Author

I'm merging this EOD if there's no reviews.

@lidavidm
Copy link
Member Author

lidavidm commented Mar 25, 2024

#1663 for getting rid of Status, #1664 for general docs and testing

@lidavidm lidavidm merged commit 6ca22c2 into apache:main Mar 25, 2024
76 checks passed
@lidavidm lidavidm deleted the sqlite-driverbase branch March 25, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants