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

Issue #341: add Crate support #342

Closed
wants to merge 4 commits into from
Closed

Issue #341: add Crate support #342

wants to merge 4 commits into from

Conversation

inge4pres
Copy link

@inge4pres inge4pres commented Feb 10, 2017

This PR introduces an additional dialect to support crate.io as per issues/341

@nelsam
Copy link
Member

nelsam commented Aug 30, 2017

Apologies for the 6 month delay; this looks solid, I'm just going to try to make sure that it's tested in CI before merging

@inge4pres
Copy link
Author

Hi :) no worries at all! Thank you for getting in touch

@notzippy
Copy link

Any updates on this ? I have an application in need of this as well :-D

@nelsam
Copy link
Member

nelsam commented Oct 21, 2017

I'm working on it now; just need to figure out how to get travis to spin up a crate DB.

dialect_crate.go Outdated
}

func (d CrateDialect) QuoteField(f string) string {
return "'" + f + "'"
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect. Crate uses double-quotes for fields and whatnot. It should be return `"` + f + `"` .

@nelsam
Copy link
Member

nelsam commented Oct 21, 2017

I added crate to the tests and it has been failing in a few ways. The first was the quoted field problem, but even after solving that, it had failures.

I've pushed a commit to your fork that adds tests; can you work on getting the tests passing?

edit: FYI, I did not update the unit tests for the crate dialect, so they'll be failing because I changed the quote characters for QuoteField.

@inge4pres
Copy link
Author

Hi @nelsam as this PR is 7 months old, I will reset my dev environment.
I tihnk I should also pull latest changes from master to consitently release the feature to add support for Crate.

From travis.yml I can see Crate is installed using the deprecated PPA installation: this forces use of 2.0.x, while a more recent version is available through adding repositories to have 2.1.x and higher, see install-cratedb-from-ppa-legacy and install-from-deb-repo.

@nelsam
Copy link
Member

nelsam commented Oct 22, 2017

@inge4pres yeah, feel free to update the .travis.yml in whatever way is best. I have no experience with crate, I just pulled the PPA from the install script (https://install.crate.io).

There are currently no conflicts with master, but feel free to merge or rebase on top of master first and do what you need to.

@inge4pres
Copy link
Author

I went as far as making pass the majority of tests, but at a certain point I'm facing that CrateDB has no INDEX statement available.

@nelsam
Copy link
Member

nelsam commented Oct 30, 2017 via email

@aman00323
Copy link

Any update on this ?

@nelsam nelsam deleted the branch go-gorp:master October 16, 2022 19:41
@nelsam nelsam closed this Oct 16, 2022
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