X Tutup
Skip to content

Fix double quotes for KEY#46

Open
nathanweeks wants to merge 2 commits intomysql2sqlite:masterfrom
cacaodb:fix/double-quotes_constraint-comma
Open

Fix double quotes for KEY#46
nathanweeks wants to merge 2 commits intomysql2sqlite:masterfrom
cacaodb:fix/double-quotes_constraint-comma

Conversation

@nathanweeks
Copy link

@nathanweeks nathanweeks commented Oct 15, 2018

Given the following DDL:

CREATE TABLE "foo1" (
  "col1" varchar(255),
  KEY "key1" ("x"),
  CONSTRAINT "constraint1" FOREIGN KEY ("fk1") REFERENCES "foo2" ("col2")
);

mysql2sqlite emits (1) a malformed CREATE INDEX statement, and (2) a line containing only a comma before the CONSTRAINT:

PRAGMA synchronous = OFF;
PRAGMA journal_mode = MEMORY;
BEGIN TRANSACTION;
CREATE TABLE "foo1" (
  "col1" varchar(255)
,
,  CONSTRAINT "constraint1" FOREIGN KEY ("fk1") REFERENCES "foo2" ("col2")
);
CREATE INDEX "idx__" ON "" ("x");
END TRANSACTION;

This patch addresses both of these issues.

dumblob
dumblob requested changes Oct 28, 2018
View reviewed changes
mysql2sqlite
" (see INFO at the end)." )
}
if( match( $0, /`[^`]+/ ) ){
if( match( $0, /["`][^"`]+/ ) ){
Copy link
Collaborator

@dumblob dumblob Oct 28, 2018

Choose a reason for hiding this comment

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

This can't be as it matches also `whatever" or "whatever` . A better option would be something like:

Suggested change
if( match( $0, /["`][^"`]+/ ) ){
if( match( $0, /[`][^`]+/ ) || match( $0, /["][^"]/ ) ){

But the issue is, that generally supporting ANSI quotes for table names (i.e. double quotes) disables the use of double quotes for string literals which is not tested with this script and I believe it'll bring weird issues.

Could you please gather a test suite with ANSI quotes as well as a test suite without ANSI quotes, so that we can see where there are issues? I can't currently test the ANSI quotes functionality.

mysql2sqlite
/(CONSTRAINT|constraint) \".*\" (FOREIGN KEY|foreign key)/ ) ){
print ","
}
}
Copy link
Collaborator

@dumblob dumblob Oct 28, 2018

Choose a reason for hiding this comment

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

This whole removal doesn't look correct to me. I can remember there was a reason I put it there but unfortunately at that time there was no policy of "unit tests for each commit" like now and I can't quickly find the reason.

Anyone with some spare time to find why this is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dumblob dumblob dumblob requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nathanweeks @dumblob @NathanWeeks-USDA
X Tutup