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

Implemented UPDATE #832

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Implemented UPDATE #832

merged 2 commits into from
Oct 17, 2019

Conversation

Hydrocharged
Copy link
Contributor

@Hydrocharged Hydrocharged commented Oct 1, 2019

Notice: one of the tests will fail until #831 is merged in, as it is required to fix one of the bugs that that test catches.

This is an implementation of UPDATE, which brings with it a new interface. Unlike the other interfaces, this one supplies two rows, the original row, and the updated row. This decision was made so that implementations that rely on primary keys can have access to the old primary key if need be.

Besides the big change, I've also caught a bug where supplying a non-existent column to ORDER BY will cause the query to still succeed, while ignoring the ORDER BY clause. Testing against MySQL showed that an error is the proper behavior, and this has been corrected. Since the change was small, I added it here rather than making an entirely-new PR for it. This was fixed by #818.

@ajnavarro
Copy link
Contributor

@Hydrocharged #831 merged! you can rebase when you wish. Thanks!

@Hydrocharged Hydrocharged force-pushed the updates branch 3 times, most recently from 2b1e5f7 to fbdd9bf Compare October 1, 2019 19:16
@Hydrocharged
Copy link
Contributor Author

@ajnavarro It has been rebased!

@Hydrocharged Hydrocharged changed the title Implemented UPDATE and properly fail on invalid ORDER BY columns Implemented UPDATE Oct 1, 2019
@erizocosmico erizocosmico requested a review from a team October 15, 2019 09:40
Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

LGTM after CI pass. It should pass just rebasing against master.

@agarciamontoro
Copy link
Contributor

Sorry for the delay on this one, @Hydrocharged. When rebasing, build will fail because of #835. You would probably just need to pass a *sql.Context wherever it is needed.

@Hydrocharged
Copy link
Contributor Author

The *sql.Contexts have been passed around! 😄

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.

Great work, thank you!

@ajnavarro ajnavarro requested a review from a team October 17, 2019 07:42
@erizocosmico
Copy link
Contributor

Thank you for the contribution!

@erizocosmico erizocosmico merged commit 89e8320 into src-d:master Oct 17, 2019
@Hydrocharged Hydrocharged deleted the updates branch November 1, 2019 22:49
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.

4 participants