8000 sql: enable use of multi-column foreign keys by dt · Pull Request #8033 · cockroachdb/cockroach · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

sql: enable use of multi-column foreign keys #8033

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
Merged
merged 4 commits into from
Jul 27, 2016
Merged

sql: enable use of multi-column foreign keys #8033

merged 4 commits into from
Jul 27, 2016

Conversation

dt
Copy link
Member
@dt dt commented Jul 26, 2016 •
edited by petermattis
Loading

Fixes #7866

This change is Reviewable

@danhhz
Copy link
Contributor
danhhz commented Jul 26, 2016

Reviewed 5 of 5 files at r1, 2 of 2 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

sql/create.go, line 431 [r2] (raw file):

  ret.target = target

  // If a column isn't specified, attempt to default to PK.

nit: s/a column isn't/no columns are/

sql/create.go, line 433 [r2] (raw file):

  // If a column isn't specified, attempt to default to PK.
  if len(targetColNames) == 0 {
      if len(target.PrimaryIndex.ColumnNames) != len(srcCols) {

this check is redundant with one below. is the error message better enough to keep the duplication?

sql/create.go, line 439 [r2] (raw file):

  }

  targetCols := make([]sqlbase.ColumnDescriptor, len(targetColNames))

It might be worth moving this block to be a method on TableDescriptor

sql/create.go, line 478 [r2] (raw file):

  if constraintName == "" {
      constraintName = parser.Name(fmt.Sprintf("fk_%s_ref_%s", fromCols[0], target.Name))

this feels more likely to conflict than the old version, what happens if it does?

sql/create.go, line 508 [r2] (raw file):

func colNames(cols []sqlbase.ColumnDescriptor) string {
  var s bytes.Buffer
  s.WriteString("(\"")

Use backticks instead of escaping these, here and throughout this function

sql/create.go, line 336 [r3] (raw file):

  }

  // Multiple FKs from the same column would potentially result in ambiguous or

can this be moved to ValidateTable?

sql/parser/create.go, line 459 [r1] (raw file):

  }
  buf.WriteString("FOREIGN KEY ")
  buf.WriteByte('(')

Combine this with the above WriteString

sql/testdata/fk, line 306 [r3] (raw file):

CREATE TABLE refpairs (a INT, b STRING, FOREIGN KEY (a, b) REFERENCES pairs2, INDEX (b, a))

# TODO(dt): allow using a strict prefix of an index

fwiw, I need this for interleaved. fast follow?

Comments from Reviewable

@dt
Copy link
Member Author
dt commented Jul 26, 2016

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

sql/create.go, line 431 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

nit: s/a column isn't/no columns are/

Done.

sql/create.go, line 433 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

this check is redundant with one below. is the error message better enough to keep the duplication?

Seemed a little clearer since it might be strange when it says "found 3" or something, but I don't feel strongly. Done.

sql/create.go, line 439 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

It might be worth moving this block to be a method on TableDescriptor

Done.

sql/create.go, line 478 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

this feels more likely to conflict than the old version, what happens if it does?

it can't, since `fromCols[0]` participates in at-most-one FK.

sql/create.go, line 508 [r2] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

Use backticks instead of escaping these, here and throughout this function

Done.

Comments from Reviewable

@dt
Copy link
Member Author
dt commented Jul 26, 2016

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

sql/create.go, line 336 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

can this be moved to ValidateTable?

it could potentially _also_ be in Validate, but at this point, the FKs aren't in the descriptor yet, just in the `fkTargets` and we call Validate before writing the desc, but we can't add the FKs (in finalizeFKs) until after that.

Comments from Reviewable

dt added 2 commits July 26, 2016 17:29
@dt
sql/parser: parse table-level FK contraints
c356065
@dt
sql: refactor resolveColFK to support multiple cols
d0ed1fb 
Still not hooked up to any codepaths that would pass it more than a single column though
@dt
Copy link
Member Author
dt commented Jul 26, 2016

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

sql/parser/create.go, line 459 [r1] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

Combine this with the above WriteString

Done.

Comments from Reviewable

@danhhz
Copy link
Contributor
danhhz commented Jul 26, 2016

:lgtm:

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

sql/create.go, line 478 [r2] (raw file):

Previously, dt (David Taylor) wrote…

it can't, since fromCols[0] participates in at-most-one FK.

Aha!

sql/create.go, line 336 [r3] (raw file):

Previously, dt (David Taylor) wrote…

it could potentially also be in Validate, but at this point, the FKs aren't in the descriptor yet, just in the fkTargets and we call Validate before writing the desc, but we can't add the FKs (in finalizeFKs) until after that.

stick a TODO in Validate plz

Comments from Reviewable

@dt
Copy link
Member Author
dt commented Jul 27, 2016

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

sql/create.go, line 336 [r3] (raw file):

Previously, paperstreet (Daniel Harrison) wrote…

stick a TODO in Validate plz

Done.

Comments from Reviewable

@dt dt merged commit 844e419 into cockroachdb:master Jul 27, 2016
@dt dt deleted the multi branch July 27, 2016 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@danhhz danhhz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dt @danhhz
0