Skip to content

sql: enable manual control of column families #7408

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.

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

Merged
merged 1 commit into from
Jun 23, 2016

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Jun 22, 2016

  • Switch to the new FormatVersion
  • Hook up the sql syntax in CREATE TABLE
  • Update SHOW CREATE TABLE
  • Update the WideTable benchmark to take advantage of column families

name old time/op new time/op delta
WideTable1_Cockroach-8 1.88ms ± 1% 1.50ms ± 0% -20.08% (p=0.008 n=5+5)
WideTable10_Cockroach-8 5.86ms ± 1% 2.59ms ± 1% -55.77% (p=0.016 n=4+5)
WideTable100_Cockroach-8 49.1ms ± 5% 13.2ms ± 5% -73.07% (p=0.008 n=5+5)
WideTable1000_Cockroach-8 570ms ± 5% 120ms ± 3% -79.00% (p=0.008 n=5+5)

name old alloc/op new alloc/op delta
WideTable1_Cockroach-8 265kB ± 0% 163kB ± 0% -38.55% (p=0.008 n=5+5)
WideTable10_Cockroach-8 1.43MB ± 0% 0.37MB ± 0% -74.27% (p=0.008 n=5+5)
WideTable100_Cockroach-8 13.2MB ± 0% 2.4MB ± 0% -82.00% (p=0.016 n=4+5)
WideTable1000_Cockroach-8 175MB ± 0% 21MB ± 0% -87.76% (p=0.008 n=5+5)

name old allocs/op new allocs/op delta
WideTable1_Cockroach-8 2.19k ± 0% 1.91k ± 0% -12.88% (p=0.008 n=5+5)
WideTable10_Cockroach-8 6.59k ± 0% 3.96k ± 0% -39.83% (p=0.008 n=5+5)
WideTable100_Cockroach-8 50.2k ± 0% 23.9k ± 0% -52.40% (p=0.008 n=5+5)
WideTable1000_Cockroach-8 546k ± 0% 223k ± 0% -59.09% (p=0.008 n=5+5)


This change is Reviewable

@petermattis
Copy link
Collaborator

4.75x faster on the WideTable1000 benchmark. Not bad. Not bad at all.


Review status: 0 of 14 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@RaduBerinde
Copy link
Member

Awesome!
:lgtm_strong:


Review status: 0 of 14 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


sql/show.go, line 155 [r1] (raw file):

      )
  }
  for _, fam := range desc.Families {

Should we omit single-column families from the output? (Creating a table with and without explicitly specifying single-column families should be pretty much the same thing right?)


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 14 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


sql/testdata/family, line 10 [r1] (raw file):

  d INT,
  FAMILY f1 (a, b),
  FAMILY f2 (c, d)

Do you have any test cases for the automatically generated family names? That is, for something like FAMILY (c, d)?


Comments from Reviewable

@danhhz
Copy link
Contributor Author

danhhz commented Jun 23, 2016

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


sql/show.go, line 155 [r1] (raw file):

Previously, RaduBerinde wrote…

Should we omit single-column families from the output? (Creating a table with and without explicitly specifying single-column families should be pretty much the same thing right?)

If a table has two single column families and you use the output of SHOW CREATE TABLE to make it again in a later version of cockroach, they may be placed in the same family by the upcoming heuristics. I'd rather err on the side of making this as roundtrippable as possible.

sql/testdata/family, line 10 [r1] (raw file):

Previously, petermattis (Peter Mattis) wrote…

Do you have any test cases for the automatically generated family names? That is, for something like FAMILY (c, d)?

Not end to end. Done.

Comments from Reviewable

- Switch to the new FormatVersion
- Hook up the sql syntax in CREATE TABLE
- Update SHOW CREATE TABLE
- Update the WideTable benchmark to take advantage of column families

name                       old time/op    new time/op    delta
WideTable1_Cockroach-8       1.88ms ± 1%    1.50ms ± 0%  -20.08%  (p=0.008 n=5+5)
WideTable10_Cockroach-8      5.86ms ± 1%    2.59ms ± 1%  -55.77%  (p=0.016 n=4+5)
WideTable100_Cockroach-8     49.1ms ± 5%    13.2ms ± 5%  -73.07%  (p=0.008 n=5+5)
WideTable1000_Cockroach-8     570ms ± 5%     120ms ± 3%  -79.00%  (p=0.008 n=5+5)

name                       old alloc/op   new alloc/op   delta
WideTable1_Cockroach-8        265kB ± 0%     163kB ± 0%  -38.55%  (p=0.008 n=5+5)
WideTable10_Cockroach-8      1.43MB ± 0%    0.37MB ± 0%  -74.27%  (p=0.008 n=5+5)
WideTable100_Cockroach-8     13.2MB ± 0%     2.4MB ± 0%  -82.00%  (p=0.016 n=4+5)
WideTable1000_Cockroach-8     175MB ± 0%      21MB ± 0%  -87.76%  (p=0.008 n=5+5)

name                       old allocs/op  new allocs/op  delta
WideTable1_Cockroach-8        2.19k ± 0%     1.91k ± 0%  -12.88%  (p=0.008 n=5+5)
WideTable10_Cockroach-8       6.59k ± 0%     3.96k ± 0%  -39.83%  (p=0.008 n=5+5)
WideTable100_Cockroach-8      50.2k ± 0%     23.9k ± 0%  -52.40%  (p=0.008 n=5+5)
WideTable1000_Cockroach-8      546k ± 0%      223k ± 0%  -59.09%  (p=0.008 n=5+5)
@danhhz danhhz merged commit 877da29 into cockroachdb:master Jun 23, 2016
@danhhz danhhz deleted the family_enable branch June 23, 2016 21:19
@spencerkimball
Copy link
Member

👍 Nice!!!

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

Successfully merging this pull request may close these issues.

5 participants