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.

Sign up for GitHub

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

Jump to bottom

ddl: disallow alter table on view #8890

Merged
merged 1 commit into from
Jan 16, 2019
Merged

ddl: disallow alter table on view #8890

merged 1 commit into from
Jan 16, 2019

Conversation

AndrewDi
Copy link
Contributor

@AndrewDi AndrewDi commented Dec 30, 2018 •
edited by c4pt0r
Loading

What problem does this PR solve?

Modify ALTER TABLE to add Table Type check,

What is changed and how it works?

ref proposal Proposal: Implement View

Check List

Tests

  • Unit test

This change is Reviewable

@AndrewDi AndrewDi mentioned this pull request Dec 30, 2018
Closed
19 tasks
@AndrewDi AndrewDi force-pushed the view/alter_table_with_view branch 3 times, most recently from f2e7520 to afa59db Compare December 31, 2018 14:41
@zz-jason zz-jason added contribution This PR is from a community contributor. component/DDL-need-LGT3 labels Jan 2, 2019
winkyao
winkyao reviewed Jan 2, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

reset LGTM

winkyao
winkyao reviewed Jan 2, 2019
View reviewed changes
ddl/ddl_api.go Outdated Show resolved Hide resolved
@winkyao
Copy link
Contributor

winkyao commented Jan 2, 2019

@AndrewDi Please address the comment, then the ci will be fixed.

winkyao
winkyao reviewed Jan 2, 2019
View reviewed changes
ddl/ddl_api.go Show resolved Hide resolved
winkyao
winkyao reviewed Jan 2, 2019
View reviewed changes
ddl/ddl_api.go Outdated Show resolved Hide resolved
@winkyao
Copy link
Contributor

winkyao commented Jan 2, 2019

@crazycs520 @zimulala PTAL

@AndrewDi
Copy link
Contributor Author

AndrewDi commented Jan 2, 2019

/run-all-tests

winkyao
winkyao reviewed Jan 4, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Jan 4, 2019

@AndrewDi Please fix ci

@codecov-io
Copy link

codecov-io commented Jan 4, 2019 •
edited
Loading

Codecov Report

Merging #8890 into master will increase coverage by 0.16%.
The diff coverage is 59.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8890      +/-   ##
==========================================
+ Coverage   66.98%   67.15%   +0.16%     
==========================================
  Files         372      372              
  Lines       76969    76949      -20     
==========================================
+ Hits        51557    51672     +115     
+ Misses      20799    20650     -149     
- Partials     4613     4627      +14
Impacted Files Coverage Δ
ddl/ddl.go 89.53% <100%> (ø) ⬆️
ddl/ddl_api.go 75.4% <53.84%> (+0.76%) ⬆️
infoschema/infoschema.go 78.94% <80%> (+0.03%) ⬆️
store/tikv/lock_resolver.go 41.7% <0%> (-0.95%) ⬇️
expression/schema.go 94.11% <0%> (-0.85%) ⬇️
executor/join.go 78.38% <0%> (-0.53%) ⬇️
ddl/delete_range.go 78.3% <0%> (+3.17%) ⬆️
util/systimemon/systime_mon.go 100% <0%> (+20%) ⬆️
util/filesort/filesort.go 76.48% <0%> (+35.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f522de2...1055629. Read the comment docs.

@AndrewDi
Copy link
Contributor Author

AndrewDi commented Jan 4, 2019

/run-all-tests

alivxxx
alivxxx reviewed Jan 4, 2019
View reviewed changes
ddl/db_change_test.go Outdated
@@ -847,7 +848,7 @@ func (s *testStateChangeSuite) TestParallelDDLBeforeRunDDLJob(c *C) {
intercept := &ddl.TestInterceptor{}
firstConnID := uint64(1)
finishedCnt := int32(0)
interval := 5 * time.Millisecond
interval := 2 * time.Millisecond
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change it?

ddl/db_change_test.go Show resolved Hide resolved
crazycs520
crazycs520 reviewed Jan 7, 2019
View reviewed changes
ddl/ddl.go Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jan 7, 2019

PTAL @zimulala

XuHuaiyu
XuHuaiyu reviewed Jan 7, 2019
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

ciscoxll
ciscoxll reviewed Jan 7, 2019
View reviewed changes
ddl/db_change_test.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Jan 9, 2019
View reviewed changes
ddl/db_change_test.go Show resolved Hide resolved
infoschema/infoschema.go Outdated Show resolved Hide resolved
@AndrewDi
Copy link
Contributor Author

@winkyao @XuHuaiyu What now?

@AndrewDi
Copy link
Contributor Author

@zz-jason PTAL

@AndrewDi
Copy link
Contributor Author

/run-all-tests

crazycs520
crazycs520 reviewed Jan 16, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor

@AndrewDi Please fix conflicts.

@crazycs520 crazycs520 added the status/LGT3 The PR has already had 3 LGTM. label Jan 16, 2019
@ngaut
Copy link
Member

ngaut commented Jan 16, 2019

/run-all-tests

@AndrewDi
Copy link
Contributor Author

/run-all-tests

@ngaut ngaut merged commit 59c7b69 into pingcap:master Jan 16, 2019
@AndrewDi AndrewDi deleted the view/alter_table_with_view branch January 17, 2019 12:40
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
@xhebox xhebox mentioned this pull request Feb 28, 2022
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@ciscoxll ciscoxll ciscoxll left review comments

@alivxxx alivxxx alivxxx left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@winkyao winkyao winkyao left review comments

@zz-jason zz-jason zz-jason left review comments

@zimulala zimulala zimulala left review comments

@ngaut ngaut ngaut approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.