nineinchnick
commented
5 months ago drivers.CopyWithInsert is used in 11 DBs. drivers_test covers 4 of them - mysql, trino, sqlserver, and csvq. clickhouse_test covers 1 more. Is more testing needed to merge this PR?
These are integration tests, and multiple DBs are tested here to avoid copying the boilerplate code that manages the test containers. I'm ok with not having the best coverage, adding more tests should be independent of improving this code.
TestWriter doesn't work for me. Part of the reason is that the test pulls schema from the main branch of https://github.com/jOOQ/sakila but the schema there changed since the test was last updated. TestWriter still fails because of whitespace differences even if if I use a specific commit in SCHEMA_URL value. Can I ignore those issues to keep the PR focused?
Yup, if the tests are not robust enough, we can improve that later. We should at least pin the jOOQ repo to a specific commit, and/or update the expected output in tests.
Unlike the existing DBs in drivers_test, csvq can't run in a container which required new code which may be considered ugly. There are a bunch of if db.RunOptions != nil . Also csvq isn't prepopulated with a schema like the other DBs. Alternatives like using a dedicated flag in the drivers_test Database struct or even an interface with multiple implementations (e.g. ContainerDB, InProcessDB) aren't clearly better. Tell me if you have a preference.
Your current solution is ok, I don't have a strong preference. Maybe we can have some CSV file committed in the repo to be used with csvq?
Thanks for all the improvements!
kenshaw
This PR applies the fixes from Clickhouse copy PR - #457 - to the common copy implementation as suggested by @nineinchnick in the discussion there.
As discussed, this PR also extends drivers_test to one more database that uses the common copy (csvq). The extended test failed before the fix and passes after it. This PR also adds a couple of convenience improvements to drivers_test:
Finally, the PR removes the custom Clickhouse copy implementation from #457 since the common copy now includes the same fixes. clickhouse_test.go, extended in #457, still passes.
Closes #427