sql: Adding support for int2, int8, float4 and float8 to be in line with postgres support of numerical types. #16720

m-schneider · 2017-06-26T21:25:50Z

Before this change we didn't support any of the types mentioned above.

After this change we'll support those types as alias and we will check
for overflow on INSERT and UPDATE. We currently won't support type checking
on mathematical operations, inline with how decimals are currently implemented.
Support for those operations would require a change to Datums, which is outside
of the scope for this change.

Closes #12481
Closes #14493

cockroach-teamcity · 2017-06-26T21:25:55Z

This change is

knz · 2017-06-27T10:59:12Z

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

pkg/sql/logictest/testdata/logic_test/table, line 309 at r1 (raw file):

statement ok
CREATE TABLE test.alltypes (
  a BOOL,

I'm really sorry that you had to perform this renumbering of the variables, and I am sorry in advance for the sentence that follows,

but please do it just once more, so that nobody else will have to do it again in the future: rename all the columns with the form c+type, for example cbool, cint, cint2 etc.
You can automate a lot of this with a regular expression for example s/([^ ]*) ([^,]*),/c\2 \2/g

Then sort the definitions in alphabetical order.

pkg/sql/parser/col_types.go, line 97 at r1 (raw file):

// Pre-allocated immutable integer column types.
var (
	intColTypeBit         = &IntColType{Name: "BIT", N: 1, ImplicitWidth: true}

Huh I now see this parameter "N". Why not reuse this for int types throughout instead of "precision"?
Or perhaps also use "precision" for type BIT.

pkg/sql/parser/col_types.go, line 124 at r1 (raw file):

	Name          string
	N             int
	Precision     int

Yeah I think what "N" does is exactly what you need here, no need for a new field.

Also as revealed in package sqlbase the right name for this field is neither "N" or "Precision" but "Width".
Currently we have there col.Type.Width = int32(t.N) this would be best written as col.Type.Width = int32(t.Width)

Note that this comment is compatible with the concepts in postgres: in pg only types numeric and float have a "precision"; the integer types have a "size".

pkg/sql/parser/col_types.go, line 147 at r1 (raw file):

	floatColTypeReal   = &FloatColType{Name: "REAL"}
	floatColTypeFloat  = &FloatColType{Name: "FLOAT"}
	floatColTypeFloat4 = &FloatColType{Name: "FLOAT4", Prec: 4, PrecSpecified: true}

There's a conceptual mistake here:
FLOAT4 and FLOAT8 differ in width (single vs double precision) - see https://www.postgresql.org/docs/9.6/static/datatype.html#DATATYPE-TABLE
FLOAT(4) and FLOAT(8) are both single precision, whereas double precision is selected with FLOAT(25) to FLOAT(54) - see https://www.postgresql.org/docs/9.6/static/datatype-numeric.html#DATATYPE-FLOAT

Type name	Width (conceptual storage size, also determines max exponent)	Guaranteed result precision (in bits, determines size of mantissa)	Notes
real	32	24	we don't support this correctly yet but we should
float	64	53
float4	32	24	synonym to `real`
float8	64	24	synonym to `float`
float(4)	32	4
float(8)	32	8
float(24)	32	24
float(25)	64	25

So you need two separate fields "width" and "prec" in FloatColType and handle them properly here, in Format and in computations.

pkg/sql/sqlbase/table.go, line 1641 at r1 (raw file):

				}
			}
			if col.Type.Precision > 0 {

look 5 lines above this is equivalent to what's done with the field width

Comments from Reviewable

knz · 2017-06-27T11:03:25Z

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

pkg/sql/parser/col_types.go, line 147 at r1 (raw file):

Previously, knz (kena) wrote…

There's a conceptual mistake here:
FLOAT4 and FLOAT8 differ in width (single vs double precision) - see https://www.postgresql.org/docs/9.6/static/datatype.html#DATATYPE-TABLE
FLOAT(4) and FLOAT(8) are both single precision, whereas double precision is selected with FLOAT(25) to FLOAT(54) - see https://www.postgresql.org/docs/9.6/static/datatype-numeric.html#DATATYPE-FLOAT

Type name Width (conceptual storage size, also determines max exponent) Guaranteed result precision (in bits, determines size of mantissa) Notes

real 32 24 we don't support this correctly yet but we should

float 64 53

float4 32 24 synonym to real

float8 64 24 synonym to float

float(4) 32 4

float(8) 32 8

float(24) 32 24

float(25) 64 25

So you need two separate fields "width" and "prec" in FloatColType and handle them properly here, in Format and in computations.

note that in computations perhaps right now we don't need to do anything, because we do not have separate DFloat32 and DFloat64. But the correct way would be to have both and use the width field to select one or another (https://stackoverflow.com/questions/16889042/postgresql-what-is-the-difference-between-float1-and-float24#16889190)

Comments from Reviewable

m-schneider · 2017-06-27T15:55:51Z

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

pkg/sql/logictest/testdata/logic_test/table, line 309 at r1 (raw file):

Previously, knz (kena) wrote…

I'm really sorry that you had to perform this renumbering of the variables, and I am sorry in advance for the sentence that follows,

but please do it just once more, so that nobody else will have to do it again in the future: rename all the columns with the form c+type, for example cbool, cint, cint2 etc.
You can automate a lot of this with a regular expression for example s/([^ ]*) ([^,]*),/c\2 \2/g

Then sort the definitions in alphabetical order.

Column Selection mode FTW!

pkg/sql/parser/col_types.go, line 97 at r1 (raw file):

Previously, knz (kena) wrote…

Huh I now see this parameter "N". Why not reuse this for int types throughout instead of "precision"?
Or perhaps also use "precision" for type BIT.

I changed both to width.

pkg/sql/parser/col_types.go, line 124 at r1 (raw file):

Previously, knz (kena) wrote…

Yeah I think what "N" does is exactly what you need here, no need for a new field.

Also as revealed in package sqlbase the right name for this field is neither "N" or "Precision" but "Width".
Currently we have there col.Type.Width = int32(t.N) this would be best written as col.Type.Width = int32(t.Width)

Note that this comment is compatible with the concepts in postgres: in pg only types numeric and float have a "precision"; the integer types have a "size".

Changed both to width.

pkg/sql/sqlbase/table.go, line 1641 at r1 (raw file):

Previously, knz (kena) wrote…

look 5 lines above this is equivalent to what's done with the field width

I see what you mean about bit strings, but the definition for integer types is here:
https://www.postgresql.org/docs/9.4/static/datatype-numeric.html#DATATYPE-INT and this function is inline with that.

Comments from Reviewable

knz · 2017-06-27T16:07:07Z

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

pkg/sql/logictest/testdata/logic_test/table, line 309 at r1 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

Column Selection mode FTW!

👍
Thank you!

pkg/sql/sqlbase/table.go, line 1641 at r1 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

I see what you mean about bit strings, but the definition for integer types is here:
https://www.postgresql.org/docs/9.4/static/datatype-numeric.html#DATATYPE-INT and this function is inline with that.

actually I hadn't seen, but the math below is ... inelegant. There's a poor trade-off here: the simple check above would be much easier on the eye and the mind of future maintainers than trying to comprehend your formula below, considering it must be first extended with and extensive comment in which you would need to explain why the math happens to be correct, although there are many reasons why it may seem wrong at first sight.
you can extend the math above to work for both BIT (an unsigned type) and INT(N) (a signed type, where either one of two loops must be used to compute the magnitude depending on the sign). Then you can replace the message bit string too long for type %s (column %q) by integer out of range for type %s (column %q), and that would be correct.

Comments from Reviewable

knz · 2017-06-27T16:10:13Z

Just for the sake of clarity, some of the many reasons why the math appears wrong:

it's quite bad to invoke a FP function (math.Pow) for something that should be doable in pure integer arithmetic
int -> float conversion is inexact for numbers larger than 2^53, so it's not clear why the formula works with a width field value larger than 53

m-schneider · 2017-06-27T18:16:46Z

FP function removed.

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

pkg/sql/sqlbase/table.go, line 1641 at r1 (raw file):

Previously, knz (kena) wrote…

actually I hadn't seen, but the math below is ... inelegant. There's a poor trade-off here: the simple check above would be much easier on the eye and the mind of future maintainers than trying to comprehend your formula below, considering it must be first extended with and extensive comment in which you would need to explain why the math happens to be correct, although there are many reasons why it may seem wrong at first sight.

you can extend the math above to work for both BIT (an unsigned type) and INT(N) (a signed type, where either one of two loops must be used to compute the magnitude depending on the sign). Then you can replace the message bit string too long for type %s (column %q) by integer out of range for type %s (column %q), and that would be correct.

I simplified the code for both bits and integers. I don't quite understand the need for the loop for bits. Using a loop for negative numbers would just be incorrect because Go uses 2s compliment to store negative numbers so our internal int64 representation would overflow for any negative number by definition.

Comments from Reviewable

knz · 2017-06-27T22:01:36Z

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

pkg/sql/parser/col_types.go, line 171 at r3 (raw file):

func (node *FloatColType) Format(buf *bytes.Buffer, f FmtFlags) {
	buf.WriteString(node.Name)
	if node.Prec > 0 && node.Width == 0 {

I think you should remove the second part of the conditional.
FLOAT4(10) is actually sensical and perhaps useful.

pkg/sql/parser/col_types_test.go, line 45 at r3 (raw file):

		{"DOUBLE PRECISION", &FloatColType{Name: "DOUBLE PRECISION"}},
		{"FLOAT", &FloatColType{Name: "FLOAT"}},
		{"FLOAT4", &FloatColType{Name: "FLOAT4", Prec: 24, Width: 16, PrecSpecified: true}},

Drop "Prec" (using the default here)
Width:32

pkg/sql/parser/col_types_test.go, line 46 at r3 (raw file):

		{"FLOAT", &FloatColType{Name: "FLOAT"}},
		{"FLOAT4", &FloatColType{Name: "FLOAT4", Prec: 24, Width: 16, PrecSpecified: true}},
		{"FLOAT8", &FloatColType{Name: "FLOAT8", Prec: 24, Width: 32, PrecSpecified: true}},

Drop "Prec" (using the default)
Width: 64

pkg/sql/parser/col_types_test.go, line 47 at r3 (raw file):

		{"FLOAT4", &FloatColType{Name: "FLOAT4", Prec: 24, Width: 16, PrecSpecified: true}},
		{"FLOAT8", &FloatColType{Name: "FLOAT8", Prec: 24, Width: 32, PrecSpecified: true}},
		{"FLOAT(4)", &FloatColType{Name: "FLOAT", Prec: 4, PrecSpecified: true}},

Add a test for FLOAT4(10) and FLOAT8(30)

pkg/sql/sqlbase/table.go, line 1641 at r1 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

I simplified the code for both bits and integers. I don't quite understand the need for the loop for bits. Using a loop for negative numbers would just be incorrect because Go uses 2s compliment to store negative numbers so our internal int64 representation would overflow for any negative number by definition.

Yes you are right. See my other comment for the followup.

pkg/sql/sqlbase/table.go, line 1621 at r3 (raw file):

		if v, ok := parser.AsDInt(val); ok {
			if col.Type.Width > 0 {
				if col.Type.VisibleType == ColumnType_NONE {

Scratch this entire conditional and its else branch. Also beware in the code you wrote if Type.Width = 64 then width = 63 then 1<<width=(-2^63) so 1<<width-1 is undefined, so your formula is still tricky to get right. I think this works:

if col.Type.Width > 0 {
   shifted := v >> uint(col.Type.Width-1)
   if (v >= 0 && shifted != 0) || (v < 0 && shifted != -1) {
       return fmt.Errorf("integer out of range for type %s (column %q)", col.Type.VisibleType, col.Name)
   }
}

Add tests for the boundary values: all combinations of Width = 0, 1, 63, 64, values = 0, 1, -1, MaxInt64, MinInt64.

Comments from Reviewable

m-schneider · 2017-06-28T17:33:12Z

Review status: 5 of 12 files reviewed at latest revision, 7 unresolved discussions.

pkg/sql/parser/col_types.go, line 147 at r1 (raw file):

Previously, knz (kena) wrote…

note that in computations perhaps right now we don't need to do anything, because we do not have separate DFloat32 and DFloat64. But the correct way would be to have both and use the width field to select one or another (https://stackoverflow.com/questions/16889042/postgresql-what-is-the-difference-between-float1-and-float24#16889190)

So should we do anything extra in this PR to support all of this correctly, or wait for the change at the datum level when we support float32 and float64.

pkg/sql/parser/col_types.go, line 171 at r3 (raw file):

Previously, knz (kena) wrote…

I think you should remove the second part of the conditional.
FLOAT4(10) is actually sensical and perhaps useful.

Removed!

pkg/sql/parser/col_types_test.go, line 45 at r3 (raw file):

Previously, knz (kena) wrote…

Drop "Prec" (using the default here)
Width:32

Dropped

pkg/sql/parser/col_types_test.go, line 46 at r3 (raw file):

Previously, knz (kena) wrote…

Drop "Prec" (using the default)
Width: 64

Dropped.

pkg/sql/parser/col_types_test.go, line 47 at r3 (raw file):

Previously, knz (kena) wrote…

Add a test for FLOAT4(10) and FLOAT8(30)

I'm not convinced that we should add support for that. As of now those two string won't be parsed by sql.go.

pkg/sql/sqlbase/table.go, line 1621 at r3 (raw file):

Previously, knz (kena) wrote…

Scratch this entire conditional and its else branch. Also beware in the code you wrote if Type.Width = 64 then width = 63 then 1<<width=(-2^63) so 1<<width-1 is undefined, so your formula is still tricky to get right. I think this works:
if col.Type.Width > 0 {
   shifted := v >> uint(col.Type.Width-1)
   if (v >= 0 && shifted != 0) || (v < 0 && shifted != -1) {
       return fmt.Errorf("integer out of range for type %s (column %q)", col.Type.VisibleType, col.Name)
   }
}
Add tests for the boundary values: all combinations of Width = 0, 1, 63, 64, values = 0, 1, -1, MaxInt64, MinInt64.

I have those tests in Scale already for int2, int4 and the bit strings. I also needed to add another check for BITs specifically because they're unsigned so using the same check would fail.

Comments from Reviewable

knz · 2017-06-28T17:54:11Z

Reviewed 2 of 2 files at r4, 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/sql/parser/col_types.go, line 147 at r1 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

So should we do anything extra in this PR to support all of this correctly, or wait for the change at the datum level when we support float32 and float64.

Up to you. However I do still believe the two types here should be initialized with Width 32 and 64 respectively not 16 and 32.

pkg/sql/parser/col_types_test.go, line 45 at r3 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

Dropped

Width: 32

pkg/sql/parser/col_types_test.go, line 46 at r3 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

Dropped.

Width: 64

pkg/sql/parser/col_types_test.go, line 47 at r3 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

I'm not convinced that we should add support for that. As of now those two string won't be parsed by sql.go.

Ack. I'm good with that.

pkg/sql/sqlbase/table.go, line 1621 at r3 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

I have those tests in Scale already for int2, int4 and the bit strings. I also needed to add another check for BITs specifically because they're unsigned so using the same check would fail.

Ack, thanks!

Comments from Reviewable

m-schneider · 2017-06-28T18:04:46Z

Review status: 10 of 12 files reviewed at latest revision, 5 unresolved discussions.

pkg/sql/parser/col_types.go, line 147 at r1 (raw file):

Previously, knz (kena) wrote…

Up to you. However I do still believe the two types here should be initialized with Width 32 and 64 respectively not 16 and 32.

Fixed!

pkg/sql/parser/col_types_test.go, line 45 at r3 (raw file):

Previously, knz (kena) wrote…

Width: 32

That's right thanks!

pkg/sql/parser/col_types_test.go, line 46 at r3 (raw file):

Previously, knz (kena) wrote…

Width: 64

Don.e

Comments from Reviewable

knz · 2017-06-28T18:11:00Z

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/sql/parser/col_types.go, line 144 at r6 (raw file):

// Pre-allocated immutable float column types.
var (
	floatColTypeReal   = &FloatColType{Name: "REAL"}

Why not populate Width properly for every FLOAT type?

pkg/sql/sqlbase/structured.go, line 1722 at r6 (raw file):

		}
	case ColumnType_FLOAT:
		if c.Precision > 0 && c.VisibleType == ColumnType_NONE {

I still think you do not need the 2nd part of this conditional, even if you decide to not support the corresponding input syntax.

Comments from Reviewable

m-schneider · 2017-06-28T18:32:54Z

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

pkg/sql/parser/col_types.go, line 144 at r6 (raw file):

Previously, knz (kena) wrote…

Why not populate Width properly for every FLOAT type?

Sure.

pkg/sql/sqlbase/structured.go, line 1722 at r6 (raw file):

Previously, knz (kena) wrote…

I still think you do not need the 2nd part of this conditional, even if you decide to not support the corresponding input syntax.

Agreed, this is before I was using default precision for the new types.

Comments from Reviewable

knz · 2017-06-28T18:34:43Z

Reviewed 4 of 4 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

jordanlewis · 2017-06-28T19:17:10Z

Nice work so far! There is a little bit more to be done to make this complete, however.

The missing feature is related to type identifiers on pgwire. With this patch, even though we can create tables with e.g. INT2columns, the type of the column will still be reported to Postgres wire clients as int8. Additionally, the metadata reflected in pg_catalog.pg_attribute will be incorrectly reported as int8.

To demonstrate this problem, observe the difference between the following queries:

CockroachDB:

root@:26257/test> CREATE TABLE types(a int8, b int2);
CREATE TABLE
root@:26257/test> SELECT attname, atttypid, typname FROM pg_attribute a JOIN pg_type t ON a.atttypid=t.oid WHERE attrelid = 'types'::REGCLASS;
+---------+----------+---------+
| attname | atttypid | typname |
+---------+----------+---------+
| a       |       20 | int8    |
| b       |       20 | int8    |
+---------+----------+---------+
(2 rows)

Postgres:

jordan=# CREATE TABLE types(a int8, b int2);
CREATE TABLE
jordan=# SELECT attname, atttypid, typname FROM pg_attribute a JOIN pg_type t ON a.atttypid=t.oid WHERE attrelid = 'types'::REGCLASS;
 attname  | atttypid | typname
----------+----------+---------
 a        |       20 | int8
 b        |       21 | int2
 tableoid |       26 | oid
 ctid     |       27 | tid
 xmin     |       28 | xid
 xmax     |       28 | xid
 cmin     |       29 | cid
 cmax     |       29 | cid
(8 rows)

(Ignore the bottom 6 rows, those are Postgres internals that we don't care about).

You can see the difference - Postgres reflects a different atttypid in pg_attribute depending on the type of the column. We report both columns as an int8.

To fix this we'll need to propagate the additional column type information that you added up into the SQL type defined in sql/type.go. The main entry point to that change will be ToDatumType in structured.go.

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

pkg/sql/logictest/testdata/logic_test/scale, line 61 at r7 (raw file):

  b INT2,
  UNIQUE INDEX a (b)
)

Can you add a test that demonstrates that SHOW CREATE TABLE remembers the column types correctly? That test should probably go in a different test file, like table.

pkg/sql/logictest/testdata/logic_test/scale, line 76 at r7 (raw file):


statement error integer out of range for type INT2 \(column "b"\)
INSERT INTO tc VALUES (32768)

We might want to consider adding a test case like INSERT INTO tc VALUES (60000-59999) with a comment to demonstrate that these column type bounds are only enforced once inserted - in case someone wants to change that one day

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

    INT2 = 3;
    INT4 = 4;
    SMALLINT = 5;

I think we don't want SMALLINT here, right? Since it's equivalent to INT4? I believe for Postgres compatibility we want just INT4.

pkg/sql/sqlbase/table.go, line 1629 at r7 (raw file):

				//to attempt to store shorter or longer bit strings. bit varying
				//data is of variable length up to the maximum length n; longer
				//strings will be rejected." Bits are unsigend, so we need to

s/unsigend/unsigned/

pkg/sql/sqlbase/table.go, line 1636 at r7 (raw file):

				}

				// We're performing bounds checks inline with Go's implementation of min and max ints in Math.go.

nit: comment is too long - while you're in here could you please make it less than 80 characters? thanks!

Comments from Reviewable

m-schneider · 2017-06-28T21:44:57Z

Being tracked on #16769, but will split into another PR.

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

pkg/sql/logictest/testdata/logic_test/scale, line 61 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Can you add a test that demonstrates that SHOW CREATE TABLE remembers the column types correctly? That test should probably go in a different test file, like table.

That's already in logictest/table. It's a bit hard to see because I refactored it a little.

pkg/sql/logictest/testdata/logic_test/scale, line 76 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

We might want to consider adding a test case like INSERT INTO tc VALUES (60000-59999) with a comment to demonstrate that these column type bounds are only enforced once inserted - in case someone wants to change that one day

Done.

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think we don't want SMALLINT here, right? Since it's equivalent to INT4? I believe for Postgres compatibility we want just INT4.

SMALLINT is an alias for INT4 in Postgres, so not having it here would break the show create table behavior.

pkg/sql/sqlbase/table.go, line 1629 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

s/unsigend/unsigned/

Done.

pkg/sql/sqlbase/table.go, line 1636 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: comment is too long - while you're in here could you please make it less than 80 characters? thanks!

Done.

Comments from Reviewable

jordanlewis · 2017-06-28T21:59:28Z

Review status: 10 of 15 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/sql/logictest/testdata/logic_test/scale, line 61 at r7 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

That's already in logictest/table. It's a bit hard to see because I refactored it a little.

D'oh! Thanks.

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

SMALLINT is an alias for INT4 in Postgres, so not having it here would break the show create table behavior.

Right - but actually we're okay with SHOW CREATE TABLE returning those aliases. Postgres always reports smallint as the "sql type name" of an int4 column, regardless of how it was created. We can do the same thing, but with int4 as the canonical name.

Furthermore, only adding SMALLINT here is inconsistent, as if we wanted to go down this road we'd also have to add e.g. BIGINT and DOUBLE PRECISION.

Comments from Reviewable

knz · 2017-06-28T23:51:08Z

Review status: 10 of 15 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Right - but actually we're okay with SHOW CREATE TABLE returning those aliases. Postgres always reports smallint as the "sql type name" of an int4 column, regardless of how it was created. We can do the same thing, but with int4 as the canonical name.

Furthermore, only adding SMALLINT here is inconsistent, as if we wanted to go down this road we'd also have to add e.g. BIGINT and DOUBLE PRECISION.

"smallint" seems better as a canonical name, not because pg does it but because it's closer to standard SQL
yes I would argue real, bigint and double precision should land here eventually too

Comments from Reviewable

jordanlewis · 2017-06-28T23:56:43Z

Review status: 10 of 15 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

Previously, knz (kena) wrote…

"smallint" seems better as a canonical name, not because pg does it but because it's closer to standard SQL

yes I would argue real, bigint and double precision should land here eventually too

Agreed
The names of these enum values don't have to be the canonical names. But we shouldn't have two enum values that map to the same thing - that doesn't make sense to me! There are 3 ways of spelling int8 (bigint, int8, int64), but there should be exactly 1 VisibleType enum value for int8. This matches how Postgres works - you can spell a type many different ways, but that type always gets normalized into a single spelling for its equivalent of SHOW CREATE.

Comments from Reviewable

knz · 2017-06-29T01:06:10Z

Review status: 10 of 15 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Agreed

The names of these enum values don't have to be the canonical names. But we shouldn't have two enum values that map to the same thing - that doesn't make sense to me! There are 3 ways of spelling int8 (bigint, int8, int64), but there should be exactly 1 VisibleType enum value for int8. This matches how Postgres works - you can spell a type many different ways, but that type always gets normalized into a single spelling for its equivalent of SHOW CREATE.

aw, ok thanks for explaining. Aye, good to choose a single canonical name. I'm in favor of the standard sql names: bigint/smallint

Comments from Reviewable

m-schneider · 2017-06-29T16:41:13Z

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

pkg/sql/sqlbase/structured.proto, line 74 at r7 (raw file):

Previously, knz (kena) wrote…

aw, ok thanks for explaining. Aye, good to choose a single canonical name. I'm in favor of the standard sql names: bigint/smallint

I added a mapping from type name to canonical type in table.go, I can move it if either of you think it should go somewhere else. Int is not the postgres canonical type for int, but I left it as such because a lot of our own code uses that.

Comments from Reviewable

knz · 2017-06-29T21:11:15Z

I'm comfortable with the result here, but will defer to Jordan for a green light.

Reviewed 12 of 12 files at r8.
Review status: 14 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 1138 at r8 (raw file):

0

#statement ok

Why are these commented out? If it's intentional, add a comment above to explain why they are commented out.

pkg/sql/sqlbase/table.go, line 1639 at r8 (raw file):

				//https://www.postgresql.org/docs/9.5/static/datatype-bit.html
				//"bit type data must match the length n exactly; it is an error

nit: we usually separate the comment prefix from the text with a space.

Comments from Reviewable

jordanlewis · 2017-06-30T00:40:30Z

mod the little typo and nit comments left!

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

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 1138 at r8 (raw file):

Previously, knz (kena) wrote…

Why are these commented out? If it's intentional, add a comment above to explain why they are commented out.

Yeah this should just have above it: ## TODO(masha): #16769

pkg/sql/sqlbase/table.go, line 1629 at r7 (raw file):

Previously, m-schneider (Masha Schneider) wrote…

Done.

Hmm maybe you didn't repush these latest changes? I still see the typo and the long line unless reviewable is malfunctioning.

Comments from Reviewable

knz · 2017-06-30T18:20:13Z

something weird happened with your branch, I'm not sure you want to see the merge commits in there?

with postgres support of numerical types. Before this change we didn't support any of the types mentioned above. After this change we'll support those types as alias and we will check for overflow on INSERT and UPDATE. We currently won't support type checking on mathematical operations, inline with how decimals are currently implemented. Support for those operations would require a change to Datums, which is outside of the scope for this change. Closes cockroachdb#12481 Closes cockroachdb#14493

m-schneider · 2017-06-30T18:49:28Z

Rebase gone badly, I'm pretty sure I fixed it now.

Review status: 11 of 15 files reviewed at latest revision, 4 unresolved discussions.

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 1138 at r8 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Yeah this should just have above it: ## TODO(masha): #16769

Done.

pkg/sql/sqlbase/table.go, line 1629 at r7 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Hmm maybe you didn't repush these latest changes? I still see the typo and the long line unless reviewable is malfunctioning.

I must have reverted something on my end. Fixed now!

pkg/sql/sqlbase/table.go, line 1639 at r8 (raw file):

Previously, knz (kena) wrote…

nit: we usually separate the comment prefix from the text with a space.

Done.

Comments from Reviewable

knz · 2017-06-30T18:58:50Z

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

Comments from Reviewable

jordanlewis · 2017-06-30T19:35:12Z

🎉

Reviewed 1 of 12 files at r1, 2 of 4 files at r7, 8 of 12 files at r8, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from Reviewable

m-schneider assigned knz Jun 26, 2017

knz assigned jordanlewis Jun 27, 2017

m-schneider force-pushed the addingint4 branch 2 times, most recently from 51450b9 to 309fbaf Compare June 27, 2017 15:46

m-schneider force-pushed the addingint4 branch from 309fbaf to fe248c3 Compare June 27, 2017 18:10

m-schneider force-pushed the addingint4 branch from fe248c3 to d6adb98 Compare June 27, 2017 19:25

m-schneider force-pushed the addingint4 branch 2 times, most recently from 6cf57bf to 6912f05 Compare June 28, 2017 17:31

m-schneider force-pushed the addingint4 branch from 6912f05 to b3d17d4 Compare June 28, 2017 18:04

m-schneider force-pushed the addingint4 branch 2 times, most recently from 4fb89ec to a46cb81 Compare June 28, 2017 18:31

m-schneider force-pushed the addingint4 branch from a46cb81 to e00bb9e Compare June 28, 2017 18:54

m-schneider force-pushed the addingint4 branch 3 times, most recently from f780027 to e102474 Compare June 28, 2017 21:06

m-schneider force-pushed the addingint4 branch from e102474 to bdebbef Compare June 29, 2017 16:29

m-schneider force-pushed the addingint4 branch from bdebbef to 45f2b08 Compare June 29, 2017 21:10

m-schneider force-pushed the addingint4 branch from 45f2b08 to a8c9b05 Compare June 30, 2017 18:00

m-schneider force-pushed the addingint4 branch from d8bb7c6 to 9dcc0b1 Compare June 30, 2017 18:44

m-schneider merged commit 837b8d0 into cockroachdb:master Jun 30, 2017

m-schneider deleted the addingint4 branch June 30, 2017 19:49

jseldess mentioned this pull request Jul 5, 2017

sql: Adding support for int2, int8, float4 and float8 to be in line with postgres support of numerical types cockroachdb/docs#1661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Adding support for int2, int8, float4 and float8 to be in line with postgres support of numerical types. #16720

sql: Adding support for int2, int8, float4 and float8 to be in line with postgres support of numerical types. #16720

m-schneider commented Jun 26, 2017

cockroach-teamcity commented Jun 26, 2017

knz commented Jun 27, 2017

knz commented Jun 27, 2017

m-schneider commented Jun 27, 2017

knz commented Jun 27, 2017

knz commented Jun 27, 2017

m-schneider commented Jun 27, 2017

knz commented Jun 27, 2017

m-schneider commented Jun 28, 2017

knz commented Jun 28, 2017

m-schneider commented Jun 28, 2017

knz commented Jun 28, 2017

m-schneider commented Jun 28, 2017

knz commented Jun 28, 2017

jordanlewis commented Jun 28, 2017

m-schneider commented Jun 28, 2017

jordanlewis commented Jun 28, 2017

knz commented Jun 28, 2017

jordanlewis commented Jun 28, 2017

knz commented Jun 29, 2017

m-schneider commented Jun 29, 2017

knz commented Jun 29, 2017

jordanlewis commented Jun 30, 2017

knz commented Jun 30, 2017

m-schneider commented Jun 30, 2017

knz commented Jun 30, 2017

jordanlewis commented Jun 30, 2017

sql: Adding support for int2, int8, float4 and float8 to be in line with postgres support of numerical types. #16720

sql: Adding support for int2, int8, float4 and float8 to be in line with postgres support of numerical types. #16720

Conversation

m-schneider commented Jun 26, 2017

cockroach-teamcity commented Jun 26, 2017

knz commented Jun 27, 2017

knz commented Jun 27, 2017

m-schneider commented Jun 27, 2017

knz commented Jun 27, 2017

knz commented Jun 27, 2017

m-schneider commented Jun 27, 2017

knz commented Jun 27, 2017

m-schneider commented Jun 28, 2017

knz commented Jun 28, 2017

m-schneider commented Jun 28, 2017

knz commented Jun 28, 2017

m-schneider commented Jun 28, 2017

knz commented Jun 28, 2017

jordanlewis commented Jun 28, 2017

m-schneider commented Jun 28, 2017

jordanlewis commented Jun 28, 2017

knz commented Jun 28, 2017

jordanlewis commented Jun 28, 2017

knz commented Jun 29, 2017

m-schneider commented Jun 29, 2017

knz commented Jun 29, 2017

jordanlewis commented Jun 30, 2017

knz commented Jun 30, 2017

m-schneider commented Jun 30, 2017

knz commented Jun 30, 2017

jordanlewis commented Jun 30, 2017