sql: parse VALIDATE commands, implement VALIDATE of CHECK

[dt]

This change is

[danhhz]

Reviewed 4 of 4 files at r1, 7 of 7 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

sql/alter_table.go, line 251 [r2] (raw file):

          }
          name := string(t.Constraint)
          details, ok := info[name]

nit: "details" could have a better name. maybe "constraint"?

---

sql/alter_table.go, line 261 [r3] (raw file):

          switch details.Kind {
          case sqlbase.ConstraintTypeCheck:
              var idx int

pull this all out into a method?

---

sql/alter_table.go, line 266 [r3] (raw file):

                      break
                  }
              }

hmm, this does handle the not found case right now, but it still makes me a little uncomfortable to not check for idx == 0 here

---

sql/alter_table.go, line 273 [r3] (raw file):

              }
              rows, err := n.p.SelectClause(&parser.SelectClause{
                  Exprs: sqlbase.ColumnsSelectors(n.tableDesc.Columns),

this is only the public columns, right? you have publicAndNonPublicColumns below

---

sql/alter_table.go, line 275 [r3] (raw file):

                  Exprs: sqlbase.ColumnsSelectors(n.tableDesc.Columns),
                  From:  &parser.From{Tables: parser.TableExprs{&n.n.Table}},
                  Where: &parser.Where{Expr: &parser.NotExpr{Expr: expr}},

woah. very nice

---

sql/alter_table.go, line 276 [r3] (raw file):

                  From:  &parser.From{Tables: parser.TableExprs{&n.n.Table}},
                  Where: &parser.Where{Expr: &parser.NotExpr{Expr: expr}},
              }, nil, &parser.Limit{Count: parser.NewDInt(1)}, nil, publicAndNonPublicColumns)

i didn't see this limit clause at first. line wrapping here could be improved

---

sql/testdata/alter_table, line 134 [r3] (raw file):

statement ok
ALTER TABLE t VALIDATE CONSTRAINT check_a

test to validate a constraint that doesn't exist?

---

Comments from Reviewable

[nvanbenschoten]

Reviewed 4 of 4 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

sql/alter_table.go, line 266 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

hmm, this does handle the not found case right now, but it still makes me a little uncomfortable to not check for idx == 0 here

I agree, it seems like there should be some assertion here.

---

sql/alter_table.go, line 272 [r3] (raw file):

                  return err
              }
              rows, err := n.p.SelectClause(&parser.SelectClause{

Is there a benefit to doing this over calling planner.queryRow?

---

sql/alter_table.go, line 293 [r3] (raw file):

              }
              n.tableDesc.Checks[idx].Validity = sqlbase.ConstraintValidity_Validated
              descriptorChanged = true

I'm not sure if it matters, but do we want to set this only if n.tableDesc.Checks[idx].Validity wasnt already ConstraintValidity_Validated?

---

sql/parser/alter_table.go, line 165 [r2] (raw file):

// AlterTableValidateConstraint represents a VALIDATE CONSTRAINT command.
type AlterTableValidateConstraint struct {

nit: Should AlterTable structs in this file be ordered alphabetically?

---

sql/parser/sql.y, line 903 [r1] (raw file):

| /* EMPTY */
  {
  $$.val = ValidationDefault

Missing two spaces.

---

sql/testdata/alter_table, line 128 [r3] (raw file):

statement error validation of CHECK "a > 0" failed on row: a=-2, f=9, b=NULL, c=NULL
ALTER TABLE t VALIDATE CONSTRAINT check_a

add a test to validate a constraint that is not failing?

---

Comments from Reviewable

[dt]

Review status: all files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

sql/alter_table.go, line 261 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

pull this all out into a method?

heh, that's in the followup that makes `ADD CONSTRAINT` call this for you (if you don't specify `NOT VALID`).

---

sql/alter_table.go, line 266 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

hmm, this does handle the not found case right now, but it still makes me a little uncomfortable to not check for idx == 0 here

The not found case is theoretically impossible, since the map was generated by iterating over the desc... but I could add an explicit panic.

what's special about idx == 0 though?

---

sql/alter_table.go, line 272 [r3] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there a benefit to doing this over calling planner.queryRow?

constructing a query string just to parse it again felt a little gross to me?

---

sql/alter_table.go, line 293 [r3] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm not sure if it matters, but do we want to set this only if n.tableDesc.Checks[idx].Validity wasnt already ConstraintValidity_Validated?

There's a `continue` above it it wasn't unvalidated.

---

Comments from Reviewable

[danhhz]

Review status: all files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

sql/alter_table.go, line 266 [r3] (raw file):

Previously, dt (David Taylor) wrote…

The not found case is theoretically impossible, since the map was generated by iterating over the desc... but I could add an explicit panic.

what's special about idx == 0 though?

I guess my point is that it's theoretically impossible _now_, but who knows what code will creep in later.

I was thinkingidx == 0 as a sentinel, but that's wrong. You'd have to use -1 or something.

---

Comments from Reviewable

[dt]

Review status: 0 of 8 files reviewed at latest revision, 12 unresolved discussions, some commit checks pending.

---

sql/alter_table.go, line 251 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

nit: "details" could have a better name. maybe "constraint"?

Done.

---

sql/alter_table.go, line 261 [r3] (raw file):

Previously, dt (David Taylor) wrote…

heh, that's in the followup that makes ADD CONSTRAINT call this for you (if you don't specify NOT VALID).

Done.

---

sql/alter_table.go, line 266 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

I guess my point is that it's theoretically impossible now, but who knows what code will creep in later.

I was thinkingidx == 0 as a sentinel, but that's wrong. You'd have to use -1 or something.

Done.

---

sql/alter_table.go, line 276 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

i didn't see this limit clause at first. line wrapping here could be improved

Done.

---

sql/parser/alter_table.go, line 165 [r2] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: Should AlterTable structs in this file be ordered alphabetically?

Huh, should they? I assumed it should go near the other constraint ones.

In practice I assume most people jump into the parser.* structs via grep/guru/gocode rather than browsing them, so can't imagine it matters either way really.

---

sql/parser/sql.y, line 903 [r1] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Missing two spaces.

Done.

---

sql/testdata/alter_table, line 128 [r3] (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

add a test to validate a constraint that is not failing?

Isn't that what the one two lines below does?

---

Comments from Reviewable

[nvanbenschoten]

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

sql/alter_table.go, line 272 [r3] (raw file):

Previously, dt (David Taylor) wrote…

constructing a query string just to parse it again felt a little gross to me?

I guess, but I hate to see the details of `expandPlan`/`Start`/`Next` scattered even further throughout this code. Encapsulating logic like this as much as possible seems like the way to go, if possible. Your call though.

---

sql/testdata/alter_table, line 128 [r3] (raw file):

Previously, dt (David Taylor) wrote…

Isn't that what the one two lines below does?

yep, didnt see that!

---

Comments from Reviewable