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(c/driver/common): Add minimal C++ driver framework #1539

Merged
merged 7 commits into from
Mar 1, 2024

Conversation

paleolimbot
Copy link
Member

This bit of code has been in the R bindings for a few months...I use it to make a few throwaway test drivers that make it easier to write self-contained tests in the adbcdrivermanager package. I'm wondering if moving it into c/common could help us move towards a common code base for the Postgres and SQLite drivers and lower the barrier for others to contribute drivers (or just copy driver_base.h and write/maintain it themselves somewhere else).

ADBC drivers are beautifully simple things: if you can turn an std::string into an ArrowArrayStream, you have an ADBC driver. Unfortunately, there are a lot of boring details that are easy to get wrong between a would-be driver contributor and a fully functional ADBC driver. I see the "boring details that are easy to get wrong" are (1) remembering how to initialize the C callables in a drive, (2) options set/get (particularly with ADBC 1.1), and (3) error handling.

The draft I've included here lets you implement the minimal driver I described above as:

class SimpleDatabase : public adbc::common::DatabaseObjectBase {};

class SimpleConnection : public adbc::common::ConnectionObjectBase {};

class SimpleStatement : public adbc::common::StatementObjectBase {
 public:
  AdbcStatusCode SetSqlQuery(const char* query, AdbcError* error) {
    sql_query_ = std::string(query);
    return ADBC_STATUS_OK;
  }

  AdbcStatusCode ExecuteQuery(ArrowArrayStream* stream, int64_t* rows_affected,
                              AdbcError* error) {
    // Use sql_query_ to go do something
    return ADBC_STATUS_NOT_IMPLEMENTED;
  }

 private:
  std::string sql_query_;
};

using SimpleDriver =
    adbc::common::Driver<SimpleDatabase, SimpleConnection, SimpleStatement>;

AdbcStatusCode SimpleDriverInitFunc(int version, void* raw_driver, AdbcError* error) {
  return SimpleDriver::Init(version, raw_driver, error);
}

I'm happy to redo anything about this (including refactor the Postgres/SQLite drivers to use it) but wanted to pause before going further to make sure we actually want to do this!

Copy link

⚠️ Please follow the Conventional Commits format in CONTRIBUTING.md for PR titles.

@github-actions github-actions bot added this to the ADBC Libraries 0.10.0 milestone Feb 12, 2024
@paleolimbot paleolimbot changed the title feat(c/common): Add minimal C++ driver framework feat(c/driver/common): Add minimal C++ driver framework Feb 12, 2024
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.

I took a quick look and this is great to have. I'll probably want to take it and run with it a bit when I get a chance (e.g. options could be templated on the concrete type for some extra type safety) but that's mostly little C++ things. I'd also be interested in extending it to handle the various state tracking that's needed for drivers.

@paleolimbot
Copy link
Member Author

I will circle back and add some low-level tests, then!

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.

LGTM, though R builds are failing?

@paleolimbot
Copy link
Member Author

Ah sorry, I forgot to circle back and check CI. I'll fix the R builds and merge!

@paleolimbot
Copy link
Member Author

LGTM, though R builds are failing?

There's an strange issue with exactly how CI installs the package ( #1581 )...I added a workaround here that is at least consistent with what was happening before; however, it won't pass CI until driver_base.h exists on the main branch.

@paleolimbot paleolimbot merged commit d9d66b2 into apache:main Mar 1, 2024
46 of 67 checks passed
@paleolimbot paleolimbot deleted the base-driver branch March 1, 2024 18:57
@lidavidm
Copy link
Member

lidavidm commented Mar 1, 2024

Thanks! I'll try to take this and run with it when I get a chance (possibly after I get the Golang equivalent squared away)

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.

2 participants