Drizzle

Merge lp://qastaging/~stewart/drizzle/bug702502-part1 into lp://qastaging/drizzle/7.0

  1. bug702502-part1
  2. Merge into 7.0
Proposed by Stewart Smith
Status: Rejected
Rejected by: Stewart Smith
Proposed branch: lp://qastaging/~stewart/drizzle/bug702502-part1
Merge into: lp://qastaging/drizzle/7.0
Diff against target: 1797 lines (+661/-360)
33 files modified
drizzled/alter_drop.h (+0/-57)
drizzled/alter_info.cc (+0/-4)
drizzled/alter_info.h (+2/-15)
drizzled/base.h (+0/-8)
drizzled/create_field.cc (+88/-69)
drizzled/create_field.h (+2/-2)
drizzled/enum.h (+0/-8)
drizzled/include.am (+0/-1)
drizzled/message/alter_table.proto (+63/-0)
drizzled/message/include.am (+6/-0)
drizzled/message/table.proto (+1/-1)
drizzled/parser.cc (+10/-5)
drizzled/sql_lex.cc (+13/-0)
drizzled/sql_lex.h (+8/-0)
drizzled/sql_parse.cc (+5/-4)
drizzled/sql_yacc.yy (+35/-18)
drizzled/statement/alter_table.cc (+268/-161)
drizzled/statement/alter_table.h (+1/-1)
drizzled/statement/create_index.cc (+1/-2)
drizzled/statement/create_index.h (+1/-1)
drizzled/statement/create_table.cc (+2/-2)
drizzled/table/instance/base.cc (+12/-0)
tests/r/alter_table_build_method.result (+7/-0)
tests/r/alter_table_rename_plus.result (+53/-0)
tests/r/create_table_errors.result (+8/-0)
tests/r/type_blob.result (+1/-1)
tests/suite/regression/r/728855.result (+21/-0)
tests/suite/regression/r/728856.result (+2/-0)
tests/suite/regression/t/728855.test (+15/-0)
tests/suite/regression/t/728856.test (+3/-0)
tests/t/alter_table_build_method.test (+7/-0)
tests/t/alter_table_rename_plus.test (+14/-0)
tests/t/create_table_errors.test (+12/-0)
To merge this branch: bzr merge lp://qastaging/~stewart/drizzle/bug702502-part1
Reviewer Review Type Date Requested Status
Lee Bieber (community) Needs Fixing
Andrew Hutchings Approve
David Shrewsbury (community) Approve
Review via email: mp+52166@code.qastaging.launchpad.net

Description of the change

First part of using a protobuf message for ALTER TABLE.

This branch does *NOT* use the proto for ANY of the replication messages.

What this branch does is start to use a protobuf message internally instead of AlterInfo.

http://hudson.drizzle.org/view/Drizzle-param/job/drizzle-param/757/

To post a comment you must log in.
Revision history for this message
David Shrewsbury (dshrews) wrote :

The proto definition is a bit confusing... why aren't RENAME and ALTER KEYS listed under the Operation enum and are in their own sub-messages instead?

It doesn't seem that there is a neat/compact/easy way to determine all of the operations that a message is trying to do. It looks like in order to do this, we'd have to call has_rename(), has_alter_keys_onoff(), AND loop through the Operations list. Is that going to get more complex as we add the rest of the possible operations?

Revision history for this message
Stewart Smith (stewart) wrote :

The reason that RENAME and ALTER KEYS are currently their own thing is that for some wonderful reason you can specify them repeatedly in the ALTER TABLE syntax, but only the last one takes effect (that is, there's a maximum of 1 of these operations per ALTER TABLE).

e.g.
CREATE TABLE t1 (a int);
CREATE TABLE t2 (a int);
ALTER TABLE t1 RENAME t2, RENAME t3;

will succeed (the first RENAME is ignored).

Yes, this sucks, but is current behaviour.

Revision history for this message
David Shrewsbury (dshrews) wrote :

Ah, I see now. If we aren't going to change that behavior going forward, then I'm fine with this.

review: Approve
Revision history for this message
Brian Aker (brianaker) wrote :

I feel like we should error out on the above.

Thoughts?

Revision history for this message
David Shrewsbury (dshrews) wrote :

@Brian: I'm all in favor of a single RENAME op per statement, if that's what you're referring to. I don't know of another DB system that supports multiple (besides MySQL).

Revision history for this message
Stewart Smith (stewart) wrote :

I'd be in favour of erroring out on multiple renames or alter keys too.

However, I did only want to change one thing at a time, and not confuse implementation change with functionality change.

Revision history for this message
Andrew Hutchings (linuxjedi) :
review: Approve
Revision history for this message
Brian Aker (brianaker) wrote :

@david I argued the RENAME thing a couple of years ago, it seems that there are a lot of folks who make use of it.

Revision history for this message
Lee Bieber (kalebral-deactivatedaccount) wrote :

Text conflict in drizzled/statement/alter_table.cc
Text conflict in drizzled/statement/create_index.cc
2 conflicts encountered.

review: Needs Fixing
lp://qastaging/~stewart/drizzle/bug702502-part1 updated
2224. By Stewart Smith

merge trunk

2225. By Stewart Smith

Merge refactoring of default values work

Revision history for this message
Stewart Smith (stewart) wrote :

I'm going to create a new merge req for putting this into lp:drizzle (not elliott)

Unmerged revisions

2225. By Stewart Smith

Merge refactoring of default values work

2224. By Stewart Smith

merge trunk

2223. By Stewart Smith

merge trunk

2222. By Stewart Smith

merge trunk

2221. By Stewart Smith

remove the now unused AlterDrop class, header file and includes

2220. By Stewart Smith

remove drop_list from AlterInfo - is now only in the message::AlterTable protobuf message

2219. By Stewart Smith

use the AlterTable protobuf message list of colmuns, keys and foreign keys to drop inside the ALTER TABLE code instead of the list in alter_info

2218. By Stewart Smith

add fields, indexes and foreign keys to drop in ALTER TABLE DROP and DROP INDEX to the AlterTable protobuf message in parser

2217. By Stewart Smith

add AlterTable operations for dropping column, keys and foreign keys (by name) in the AlterTable protobuf message.

2216. By Stewart Smith

remove enum_enable_or_disable which was only used for alter_info.keys_onoff. We can just use the message::AlterTable now

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches