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

Fixes in \copy implementation for Clickhouse #457

Merged
merged 2 commits into from
Apr 17, 2024

Conversation

murfffi
Copy link
Contributor

@murfffi murfffi commented Apr 12, 2024

A couple of fixes in the Copy function for Clickhouse:

  • Don't prepare the SELECT statement that determines columns for the destination table as the Clickhouse driver omly supports preparing INSERT statements
  • De-reference the pointers populated by Scan before supplying them to arguments of Exec because Clickhouse, in some cases, doesn't accept a pointer to an arguments, instead of the argument itself. For example, this happens when source database driver doesn't support ScanType() and the value populated by Scan is interface{}.

The new automated test fails without the fixes and succeeds with them.

@murfffi
Copy link
Contributor Author

murfffi commented Apr 12, 2024

The automated test could have been either in driver_test.go or in clickhouse_test.go . The copy test in driver_test.go
expects destination DB supports RowsAffected but Clickhouse doesn't. So I made a dedicated copy test in clickhouse_test.go. I can implement the other way but the code in drivers_test.go can become convoluted.

if err != nil {
return n, fmt.Errorf("failed to scan row: %w", err)
}
//We can't use values... in Exec() below, because, in some cases, clickhouse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something that others drivers would also benefit from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think both changes apply to the general drivers.CopyWithInsert implementation. More testing will be needed though since CopyWithInsert is used in 10 drivers.

Copy link
Contributor Author

@murfffi murfffi Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about doing this in three PRs

  1. This PR fixes Clickhouse only at first
  2. I do Batch insert in \copy to DB destination #458 for Clickhouse
  3. I integrate these fixes into drivers.CopyWithInsert + batching. To exercise the fixes, extend TestCopy drivers_test.go to include a source DB that doesn't support ScanType (e.g. csvq), and a target DB that only support prepared INSERT statements (e.g. Clickhouse, Databend).

Trying to go directly to step 3 will delay the fix for \copy for Clickhouse.

drivers/clickhouse/clickhouse.go Outdated Show resolved Hide resolved
@nineinchnick nineinchnick merged commit 7663e7e into xo:master Apr 17, 2024
1 check passed
@murfffi murfffi deleted the fix/clickhouse_copy branch April 17, 2024 20:27
kenshaw added a commit to xo/homebrew-xo that referenced this pull request May 11, 2024
## What's Changed
* Fix Clickhouse Test by @murfffi in xo/usql#455
* chore: fix function name in comment by @oftenoccur in xo/usql#456
* Fixes in \copy implementation for Clickhouse by @murfffi in xo/usql#457

## New Contributors
* @murfffi made their first contribution in xo/usql#455
* @oftenoccur made their first contribution in xo/usql#456

**Full Changelog**: xo/usql@v0.18.1...v0.19.0
archlinux-github pushed a commit to archlinux/aur that referenced this pull request May 11, 2024
## What's Changed
* Fix Clickhouse Test by @murfffi in xo/usql#455
* chore: fix function name in comment by @oftenoccur in xo/usql#456
* Fixes in \copy implementation for Clickhouse by @murfffi in xo/usql#457

## New Contributors
* @murfffi made their first contribution in xo/usql#455
* @oftenoccur made their first contribution in xo/usql#456

**Full Changelog**: xo/usql@v0.18.1...v0.19.0
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