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

Dereference fix in common copy implementation #461

Closed
wants to merge 1 commit into from

Conversation

murfffi
Copy link
Contributor

@murfffi murfffi commented May 3, 2024

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:

  • leftover containers from previous runs with -cleanup=false no longer cause the test to timeout and fail if the containers are stopped (e.g. due to a system restart).
  • TestCopy testcases are separate subtests which makes it easier to see which testcase failed.

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

@murfffi murfffi marked this pull request as draft May 3, 2024 16:12
drivers/drivers_test.go Outdated Show resolved Hide resolved
drivers/drivers_test.go Outdated Show resolved Hide resolved
drivers/drivers_test.go Outdated Show resolved Hide resolved
@nineinchnick
Copy link
Member

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!

@murfffi murfffi marked this pull request as ready for review May 10, 2024 08:34
@murfffi murfffi changed the title Draft: Dereference fix in common copy implementation Dereference fix in common copy implementation May 10, 2024
@kenshaw
Copy link
Member

kenshaw commented May 10, 2024

Merged.

@kenshaw kenshaw closed this May 10, 2024
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.

does not succed to export data in csv with \copy
3 participants