Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

Implemented REPLACE #825

Merged
merged 3 commits into from
Sep 30, 2019
Merged

Implemented REPLACE #825

merged 3 commits into from
Sep 30, 2019

Conversation

Hydrocharged
Copy link
Contributor

This PR is a follow-up to #822 and includes that commit's changes as well.

Looking over the MySQL REPLACE documentation, I see that it mentions a REPLACE being either a DELETE then INSERT, or just an INSERT. I figured that there were two ways to implement this: as another interface with a unique function (Replacer with a Replace function for example), or by leveraging the already-included Insert and Delete functions from their respective interfaces. I decided to go with the latter option for the below reasons.

One point is in the reduction of code needed for someone wanting to implement the library. The more that the framework handles (without appreciable loss of freedom), the better. Then there is also the consideration of triggers. If DELETE and INSERT calls should make use of triggers (which are probably already built into the implementing Delete and Insert functions), then simply calling those as normal will allow for correct behavior by default.

The biggest downside that I could see is that Delete is now forced to check if the given row exists, and return the appropriate error if not. For some implementations, this will not make an impact on the written code nor performance, but for others it definitely will. I felt that the trade off was worth it, but I'm interesting in hearing your opinions as well.

A third potential option is to allow for a custom Replace function if provided, otherwise defaulting to DELETE + INSERT from earlier. For example:

type Replacer interface {
	Deleter
	Inserter
}

type CustomReplacer interface {
	Replace(*Context, Row) error
}

This would solve both of the problems as far as I can tell, but would not follow in the pattern established for every other keyword, and thus I decided against this.

Signed-off-by: Daylon Wilkins <daylon@liquidata.co>
Signed-off-by: Daylon Wilkins <daylon@liquidata.co>
@erizocosmico erizocosmico requested a review from a team September 25, 2019 08:06
Copy link
Contributor

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

Thank you for the hard work, @Hydrocharged! Your rationale behind the implementation seems correct to me. I just left two minor comments below.

sql/parse/parse.go Outdated Show resolved Hide resolved
sql/plan/insert.go Show resolved Hide resolved
Signed-off-by: Daylon Wilkins <daylon@liquidata.co>
@erizocosmico erizocosmico requested a review from a team September 26, 2019 07:37
Copy link
Contributor

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

🎉

@ajnavarro ajnavarro merged commit 2e82b0a into src-d:master Sep 30, 2019
@Hydrocharged Hydrocharged deleted the replace branch October 1, 2019 18:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants