Vinai/show create table check constraints #414

VinaiRachakonda · 2021-05-11T15:54:35Z

This pr allow for the printing of check constraints during "SHOW CREATE TABLE"

zachmu

Better but too many parens, see comments

zachmu · 2021-05-12T18:52:24Z

enginetest/enginetests.go

@@ -3007,7 +3018,7 @@ func TestColumnDefaults(t *testing.T, harness Harness) {
 		TestQuery(t, harness, e, "SHOW CREATE TABLE t29", []sql.Row{{"t29", "CREATE TABLE `t29` (\n" +
 			"  `pk` bigint NOT NULL,\n" +
 			"  `v1y` bigint,\n" +
-			"  `v2` bigint DEFAULT (v1y + 1),\n" +
+			"  `v2` bigint DEFAULT ((v1y + 1)),\n" +


You should strip these extra parens (just don't add them in the first place)

This is actually the correct mysql behavior.

Thanks for checking, would not have guessed this

Add a TODO to backtick quote the columns in column defaults, this will break for some things as is

zachmu · 2021-05-12T18:53:02Z

enginetest/queries.go

@@ -5395,7 +5395,8 @@ var InfoSchemaQueries = []QueryTest{
 				"  `s` varchar(20) NOT NULL COMMENT 'column s',\n" +
 				"  PRIMARY KEY (`i`),\n" +
 				"  KEY `mytable_i_s` (`i`,`s`),\n" +
-				"  UNIQUE KEY `mytable_s` (`s`)\n" +
+				"  UNIQUE KEY `mytable_s` (`s`),\n" +
+				"  CONSTRAINT `mycheck` CHECK ((`i` > -100))\n" +


Same here. You don't need to add parens to the check expression anymore, since every expression prints their own now

zachmu · 2021-05-12T18:54:59Z

enginetest/query_plans.go

@@ -26,11 +26,11 @@ var PlanTests = []QueryPlanTest{
 	{
 		Query: `SELECT t1.i FROM mytable t1 JOIN mytable t2 on t1.i = t2.i + 1 where t1.i = 2 and t2.i = 1`,
 		ExpectedPlan: "Project(t1.i)\n" +
-			" └─ IndexedJoin(t1.i = t2.i + 1)\n" +
-			"     ├─ Filter(t2.i = 1)\n" +
+			" └─ IndexedJoin((t1.i = (t2.i + 1)))\n" +


Same comment throughout here. Change the String() methods of these nodes that print expressions within parens. They don't need parens anymore

BTW after you make these changes, don't forget there's a skipped unit tests that regenerates this file for just this kind of emergency

zachmu · 2021-05-12T19:03:59Z

enginetest/script_queries.go

+					},
+				},
+			},
+			// TODO: Expression tree is Incorrect


Bad comment on my part. I should've been explained that I thought this caused a bug on the expression tree. I just double checked and it's no longer a problem

enginetest/script_queries.go

zachmu · 2021-05-12T19:07:14Z

sql/plan/show_create_table.go

@@ -243,6 +246,18 @@ func (i *showCreateTablesIter) produceCreateTableStatement(table sql.Table) (str
 		}
 	}

+	if i.checks != nil {
+		for _, check := range i.checks {
+			fmted := fmt.Sprintf("  CONSTRAINT `%s` CHECK (%s)", check.Name, check.Expr.String())


No need for parens here, as mentioned earlier

zachmu

LG, just a couple small changes for integrator compatibility necessary

zachmu · 2021-05-14T19:21:36Z

enginetest/enginetests.go

@@ -3007,7 +3018,7 @@ func TestColumnDefaults(t *testing.T, harness Harness) {
 		TestQuery(t, harness, e, "SHOW CREATE TABLE t29", []sql.Row{{"t29", "CREATE TABLE `t29` (\n" +
 			"  `pk` bigint NOT NULL,\n" +
 			"  `v1y` bigint,\n" +
-			"  `v2` bigint DEFAULT (v1y + 1),\n" +
+			"  `v2` bigint DEFAULT ((v1y + 1)),\n" +


Thanks for checking, would not have guessed this

zachmu · 2021-05-14T19:22:30Z

enginetest/enginetests.go

@@ -3007,7 +3018,7 @@ func TestColumnDefaults(t *testing.T, harness Harness) {
 		TestQuery(t, harness, e, "SHOW CREATE TABLE t29", []sql.Row{{"t29", "CREATE TABLE `t29` (\n" +
 			"  `pk` bigint NOT NULL,\n" +
 			"  `v1y` bigint,\n" +
-			"  `v2` bigint DEFAULT (v1y + 1),\n" +
+			"  `v2` bigint DEFAULT ((v1y + 1)),\n" +


Add a TODO to backtick quote the columns in column defaults, this will break for some things as is

zachmu · 2021-05-14T19:29:23Z

enginetest/queries.go

@@ -5395,7 +5395,8 @@ var InfoSchemaQueries = []QueryTest{
 				"  `s` varchar(20) NOT NULL COMMENT 'column s',\n" +
 				"  PRIMARY KEY (`i`),\n" +
 				"  KEY `mytable_i_s` (`i`,`s`),\n" +
-				"  UNIQUE KEY `mytable_s` (`s`)\n" +
+				"  UNIQUE KEY `mytable_s` (`s`),\n" +
+				"  CONSTRAINT `mycheck` CHECK (`i` > -100)\n" +


You actually can't do this to myTable, it's just too widely used. With this setup, nobody gets to use myTable unless they implement check constraints

Instead, conditionally create a new table in test_data.go only if the harness supports checks, OR

Just test this in the existing Check methods in enginetests.go (which integrators can decide to not test if they can't support it)

zachmu · 2021-05-14T19:34:10Z

enginetest/script_queries.go

@@ -755,4 +755,156 @@ var ScriptTests = []ScriptTest{
 			},
 		},
 	},
+	{


You can't put these in here for the same reason you can't change mytable: ScriptTests is very generic, meant to support most implementors. Adding the necessity of check constraints breaks that

Put this script into its own var, probably in a new file

VinaiRachakonda added 22 commits May 7, 2021 17:07

add the checks table to testdata. Missing still

8611ca6

works, need to add tests

febea90

checkpoint. still need to handle paren printing

499576c

checkpoint 2 more tests, need to fix inner parens

e1aa862

checkpoint 3 doesn twork with outer parens

0dbed61

checkpoint 4, need to restart

9edf8d7

passing tests need to refactor

23ed90c

add not enforced logic

fcb848c

add multiple checks case

45ecbd0

format

526a981

small formatting

053de26

more refactoring

d42be0b

even more refactroring

5d31b8a

more refactoring

36cf9ce

more refactoring

559e352

a lot more refactoring

a1716e2

final nits

b06dda1

add the edge case

5183565

fmt

4a3f1a7

fix tests

179e3fe

fmt

4582a25

merge

6f4995c

VinaiRachakonda changed the title ~~[wip] Vinai/show create table check constraints~~ Vinai/show create table check constraints May 12, 2021

zachmu reviewed May 12, 2021

View reviewed changes

VinaiRachakonda added 6 commits May 12, 2021 17:42

cleanup parens

ebb741d

some more fixes

0acc23e

unskip

862e64e

fix in subquery bugs

6877647

fmt and fixes

252aa28

fix todos

601e2b2

VinaiRachakonda added 4 commits May 13, 2021 10:25

fix create table tests

6a50fe9

redo the default paren printing

cef32c8

final nits

5e2924e

last nit

97f76ec

zachmu approved these changes May 14, 2021

View reviewed changes

VinaiRachakonda added 4 commits May 14, 2021 19:12

Remove checkharness and check from mytable

2c34eed

fix these tests and cleanup

da149e8

dedup tests

938576f

last nit

6d68591

VinaiRachakonda merged commit c64084a into master May 15, 2021

Hydrocharged deleted the vinai/show-create-table-check-constraints branch December 8, 2021 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vinai/show create table check constraints #414

Vinai/show create table check constraints #414

VinaiRachakonda commented May 11, 2021 •

edited

Loading

zachmu left a comment

zachmu May 12, 2021

VinaiRachakonda May 13, 2021

zachmu May 14, 2021

zachmu May 14, 2021

zachmu May 12, 2021

zachmu May 12, 2021

zachmu May 12, 2021

zachmu May 12, 2021

VinaiRachakonda May 12, 2021

zachmu May 12, 2021

zachmu left a comment

zachmu May 14, 2021

zachmu May 14, 2021

zachmu May 14, 2021

zachmu May 14, 2021

@@ @@ -755,4 +755,156 @@ var ScriptTests = []ScriptTest{ @@
               			},
               		},
               	},
+              	{

Vinai/show create table check constraints #414

Vinai/show create table check constraints #414

Conversation

VinaiRachakonda commented May 11, 2021 • edited Loading

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinaiRachakonda commented May 11, 2021 •

edited

Loading