Skip to content

vertica driver support#393

Merged
mfridman merged 2 commits intopressly:masterfrom
bobpace:vertica
Dec 5, 2022
Merged

vertica driver support#393
mfridman merged 2 commits intopressly:masterfrom
bobpace:vertica

Conversation

@bobpace
Copy link
Copy Markdown
Contributor

@bobpace bobpace commented Aug 5, 2022

  • Add VerticaDialect
  • Add vertica-sql-go dependency
  • Add !no_vertica build tag in driver_vertica.go
  • Updated README
  • Updated CLI help message
  • Test coverage

I've been using these changes successfully already with a custom build of goose to manage vertica migrations for a project. I'm willing to include test coverage similar to what has been done for other drivers if there is interest in accepting this PR. Let me know if there is any other feedback you have for me that I may have missed. Thanks!

@mfridman
Copy link
Copy Markdown
Collaborator

Seems reasonable, I have not personally used vertica, but judging by the queries it seems pretty straightforward.

I presume we can use a docker container, such as:

https://hub.docker.com/r/vertica/vertica-ce

@bobpace
Copy link
Copy Markdown
Contributor Author

bobpace commented Aug 15, 2022

I was able to get the container to work with a few modifications to reduce the startup time:

  • setting VMART_ETL_SCRIPT env variable to empty string to prevent loading VMART schema
  • mounting empty folder over /opt/vertica/packages to prevent additional package installation steps

Only one instance of Vertica can be running on a host at any time so I put all assertions into one unit test.

@thackerc
Copy link
Copy Markdown

@mfridman I'd love to see this merged. Any concerns on your end?

@mfridman
Copy link
Copy Markdown
Collaborator

No concerns with the PR itself, more concern about the ongoing maintenance.

I'm not familiar with vertica DB, so if users start filing issues/ask questions there is an additional maintenance cost to the current and future maintainers.

I'm starting to think there might be "official" and "third-party" goose support for the various databases and drivers.

@bobpace
Copy link
Copy Markdown
Contributor Author

bobpace commented Sep 19, 2022

What do you think about instead of this PR just exporting the functions on the SQLDialect interface so it can be implemented outside the goose package and offering something like:

func SetDialectThirdParty(d SQLDialect) {
	dialect = d
}

@tmm1
Copy link
Copy Markdown

tmm1 commented Nov 8, 2022

What do you think about instead of this PR just exporting the functions on the SQLDialect interface so it can be implemented outside the goose package and offering something like:

Interesting idea, but that would mean you could only use it from the golang API right, and the cli would not work?

@mfridman
Copy link
Copy Markdown
Collaborator

Hey @bobpace, if you're up for it, can you rebase (or merge) main, overall looks good to me and we can merge this in.

@bobpace
Copy link
Copy Markdown
Contributor Author

bobpace commented Dec 2, 2022

That sounds great, thanks!

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.

4 participants