All committers have accepted the CLA.

Can you file the other bugs you found? I'd really like for this to go in with the associated logic test enablement.

Sure, I'll file the other bugs.

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

sql/create.go, line 132 [r1] (raw file):
Is this the right builtin to be using? experimental_unique_int() would be faster, though the ordering might not be desirable.

sql/create.go, line 134 [r1] (raw file):
Is there precedent for the leading underscore? Perhaps just ROWID for this column name. Btw, column names are case-insensitive and we generally use lowercase.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
If you added a new column to the table via an ALTER TABLE t ADD c INT statement, then the internal columns would be a, b, _rowid, c. Subsequent insertions would not work as expected. I think you need to tag the _rowid ColumnDescriptor as auto-generated so that it can be ignored for insertions and other operations.

sql/testdata/no_primary_key, line 26 [r1] (raw file):
You can do something like SELECT length(_rowid) FROM t to verify that the _rowid column exists.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
Done.

sql/create.go, line 134 [r1] (raw file):
Done.

sql/testdata/no_primary_key, line 26 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
We should extract a constant, so that when this method is no longer "experimental", this doesn't rot.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
The test would fail if we removed the experimental_ prefix from the builtin without fixing this code. I fail to see how rot would occur.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
I'm fine with addressing this in a future PR, but it would good to add a test case demonstrating the problem to this PR.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
"Rot" was the wrong word.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

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

Is where desc.PrimaryIndex.ColumnIDs is appended inside of the AllocateIDs func. The create function could go through and manually examine all indicies to see if any are primary, but it may be better to keep that logic in one place in AllocateIDs.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
Where should I make this test case? If I make it in this PR, it'll have to be commented out with a comment explaining why. What about an open issue/PR with the breaking test case, that we can merge with the fix?

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r2] (raw file):
The address taking is indeed why you can't use a const here. I have no opinion on moving the string to a common place so it doesn't break during rename. I'm inclined to leave it as is.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
I'd add the test case to the end of this file. You don't have to comment it out, just expect the error (statement error ...) instead of success and add a TODO(mjibson): this should succeed comment.

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 130 [r1] (raw file):
I'm not sure what that has to do with anything; why does it prevent you from checking ColumnNames?

Comments from the review on Reviewable.io

Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

sql/testdata/no_primary_key, line 10 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Reviewed 1 of 2 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
hm, I thought we added this mostly for the sql tests and now it seems like we worry about performance in a way that would rather have us speed up ID generation ever so slightly, but instead have write hotspots for all such tables. I'm not invested in this, just curious why that's a good choice.

Comments from the review on Reviewable.io

LGTM

Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
I'm not sure what the right choice is here. Certainly, the user shouldn't expect any ordering of the rows if they didn't specify a primary key. ID generation speed seems of small importance. The size of the ID field (16-bytes for the UUID vs 8-bytes for an INT) seems more important, but still relatively minor.

Comments from the review on Reviewable.io

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 130 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
Suppose we could always have a version of experimental_unique_int() which reverses the bit order so that it should be distributed enough if we ever have high-performance users of this.

Comments from the review on Reviewable.io

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/testdata/no_primary_key, line 38 [r4] (raw file):
Not a big deal, but you could make the INSERT fail by giving column c the type STRING.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 3 unresolved discussions.

sql/create.go, line 132 [r1] (raw file):
Yeah. Might be worth having an internal_rowid() function.

Comments from the review on Reviewable.io

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

sql/create.go, line 132 [r1] (raw file):
I don't understand the problem with experimental_unique_int and why reversing the bit order matters.

sql/testdata/no_primary_key, line 38 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/create.go, line 132 [r1] (raw file):
experimental_unique_int is composed of a timestamp+node-id. The timestamp occupies the high bits and thus causes the generated IDs to be mostly sequential (modulo clock skew between different nodes). Sequential IDs create a hotspot since insertion of rows into a table will all be directed at the same range. If we reversed the bits the writes would be scattered and could take advantage of more write bandwidth. On the downside, reversing the bits would enforce writes being scattered so the insertion of 2 rows would necessarily hit 2 ranges. I'm not sure if there is a best answer here.

Comments from the review on Reviewable.io

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/create.go, line 132 [r1] (raw file):
Makes sense. Are there objections to merging this PR, and we can choose to change the implementation later? I don't care one way or the other, but if we don't know which is better, maybe it would work to merge this and see how things go, then change it if it's bad.

Comments from the review on Reviewable.io

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

sql/create.go, line 132 [r1] (raw file):
I have no objection to merging as-is.

Comments from the review on Reviewable.io

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io