joellubi commented on PR #1590:
URL: https://github.com/apache/arrow-adbc/pull/1590#issuecomment-1986250180
@lidavidm @zeroshade This isn't complete yet, but I've made some changes to
they way the Connection gets built and wanted to get your thoughts on this
direction.
I ended up breaking the previous large interface into several smaller ones,
so that implementors may choose whichever ones are appropriate to use. With a
single interface, there was a downside that the use of driverbase was "all or
nothing". An example of this would be if the `GetObjects` logic for a driver
doesn't fit well with the
`GetObjectsCatalogs/GetObjectsDbSchemas/GetObjectsTables` combo. It may be
easier to just write a custom `GetObjects` method from scratch but then it's
not possible to use any of the other helpers.
By breaking up the interfaces and electing to only use the ones that are
relevant, it allows implementors to mix/match custom methods with those
provided by the driverbase. I've used a builder pattern to specify the helpers
being used which makes it quite easy to discover which helpers are available to
implement. Open to feedback on this approach. The way this looks in practice:
For driver_test, all of the helpers are used to demonstrate the behavior:
```go
func (db *databaseImpl) Open(ctx context.Context) (adbc.Connection, error) {
cnxn := &connectionImpl{ConnectionImplBase:
driverbase.NewConnectionImplBase(&db.DatabaseImplBase), db: db}
bldr := driverbase.NewConnectionBuilder(cnxn)
return bldr.
WithAutocommitSetter(cnxn).
WithCurrentNamespacer(cnxn).
WithTableTypeLister(cnxn).
WithDriverInfoPreparer(cnxn).
WithDbObjectsEnumerator(cnxn).
Connection(), nil
}
```
For the flightsql_driver, `CurrentNamespacer` is not specified because
sessions are not supported (yet) and `TableTypeLister` just isn't helpful
because the data is already coming in as Arrow records, so there's not much
sense in converting back/forth to a string list.
```go
return driverbase.NewConnectionBuilder(conn).
WithDriverInfoPreparer(conn).
WithAutocommitSetter(conn).
WithDbObjectsEnumerator(conn).
Connection(), nil
```
For the snowflake driver, `DriverInfoPreparer` is not used because it
doesn't send any driver info beyond the default build info, and a custom
`GetObjects` method is used for this driver so we skip using
`DbObjectsEnumerator`.
```go
return driverbase.NewConnectionBuilder(conn).
WithAutocommitSetter(conn).
WithCurrentNamespacer(conn).
WithTableTypeLister(conn).
Connection(), nil
```
A few things to note about this approach: Each method on the builder
enforces that those methods are implemented at compile time, which is nice. In
these examples the `connectionImpl` itself implements each interface, which is
nice because we can use the common connections state in each one, but with
multiple interfaces it's now possible to start splitting some of these out into
their own structs if they can manage their own state. This may be helpful for
factoring the drivers into smaller components as they grow larger.
The names for the interfaces are kind of awkward, open to any suggestions.
What do you think of this approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
