Implement driver.QueryerContext interface

[RichardKeo]

As per the specifications, QueryerContext is an optional interface.
When it's not implemented, the driver falls back to the Queryer interface that is also optional, and eventually defaults to:

• preparing a query
• executing the statement
• closing the statement

This means that the context passed to QueryContext gets completely ignored and therefore timeouts aren't honoured.

This commit provides an implementation of the QueryerContext interface that honours the context. When the context expires or gets cancelled, the statement gets cancelled and closed, and upon completion, an error is returned.

[hawkaa]

Looks great! We're definitely building this driver much better. This is not the first interface we've implemented, and it might not be the last.

A couple of comments:

1. It seems like by implementing the interface, we need to copy-paste a lot of code from Somewhere ™️ . Where is this Somewhere? Is there any way we can use what already exists and add custom error handling just for when the context cancels?
2. Did we consider adding any tests? I know its not the favourite chore but the feedback loop of pushing to master, building a new worker and testing that worker is really long. A testing framework could help. Also if we want this code merged upstream we are most likely going to need that. We want the code upstream so the open source community can maintain together with us.

What do y

[hawkaa]

Should this be tested somehow?

[RichardKeo]

Whoops, I forgot to add code attribution for that part. This is coming from the Go codebase: https://github.com/golang/go/blob/03ac39ce5e6af4c4bca58b54d5b160a154b7aa0e/src/database/sql/ctxutil.go#L137-L146

[RichardKeo]

It seems like by implementing the interface, we need to copy-paste a lot of code from Somewhere ™️ . Where is this Somewhere? Is there any way we can use what already exists and add custom error handling just for when the context cancels?

A lot of the logic is coming from stmt.go, which we can't use directly unfortunately.
Indeed, stmt.Prepare() returns a driver.Stmt which doesn't expose any function to cancel a statement, which is what we need to propagate the cancellation to the database.

We could potentially manually build a odbc.Stmt and reuse stmt.Query() but it wouldn't really simplify the code that much I believe.

We would essentially replace

odbc/conn.go

Lines 107 to 120 in 86533c7

	err := os.Exec(dargs, c)
	if err != nil {
	errorChan <- err
	return
	}
	err = os.BindColumns()
	if err != nil {
	errorChan <- err
	return
	}
	os.usedByRows = true
	rowsChan <- &Rows{os: os}

with something like (untested code):

stmt := &Stmt{c: c, os: os, query: query}
rows, err := stmt.Query(dargs)
if err != nil {
    errorChan <- err
} else {
    rowsChan <- rows
}

But then, there's also a chance the ODBC statement gets replaced by that function too. What do you think?

Did we consider adding any tests? I know its not the favourite chore but the feedback loop of pushing to master, building a new worker and testing that worker is really long. A testing framework could help. Also if we want this code merged upstream we are most likely going to need that. We want the code upstream so the open source community can maintain together with us.

I have considered it, but I'm not so sure how to test it yet. In the repo, I can see tests mostly for MS SQL, which I don't have and therefore most likely can't test.
And if we wanted to get it merged upstream, we'd probably need to do the changes in https://github.com/duneanalytics/odbc/blob/master/api/zapi_windows.go too, and have a way to test it.

I'm open to ideas if you have any suggestions though!

[vegarsti]

This LGTM, but it would be nice with comments indicating what's copied from where. The way I understand it is that this is roughly Query, but with the call to Cancel?

[va3093]

Nice richard. I am not familiar with this code base but would it be easy to test this functionality?

[hawkaa]

I've been maintaining the odbc fork myself for a while now. The changes up until now has been rather small. There is one bug in the underlying driver that we're fixing in this repository, in this commit. Besides that, we've managed to merge most of it upstream, with the exception of #171 which wasn't well received due to poor communication from my end. I intend to fix this.

My main concern regarding this change is that I am afraid that this driver, which is an utterly important piece, becomes a house of broken windows When our new functionality has no unit tests, and the feedback loop to test it end-to-end is really long, it is tempting to keep changes small and keep breaking windows. There are two ways we can solve for this:

1. Make sure changes are merged upstream. This way we get more eyes on the codebase, and more usage with a community that reports bugs. There's also a testing suite.
2. Add more tests and improve developer experience within this repository.

I'm really bullish about 1 even though it is the hardest one. But I think we want to contribute to open source and make the world a better place one line of code at a time.

I'm happy to merge this but we should keep this in mind when we keep patching this driver.

[vegarsti]

I agree with what Håkon says, we should push for upstream changes. It might be possible to do some effort for 2), as well, to make the feeedback loop tighter. But I think it's sufficient to use a branch on core and update its reference to the module in this repository, since we have a good testing setup there, and we test queries against Spark.

[vegarsti]

Btw I really appreciate the changes you made in c8a9911, I think that made things more clear!