sql: fix the handling of integer types#29020
Conversation
|
This change is |
BramGruneir
left a comment
There was a problem hiding this comment.
Reviewed 27 of 27 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/table_test.go, line 58 at r1 (raw file):
{ "INT8", sqlbase.ColumnType{SemanticType: sqlbase.ColumnType_INT, VisibleType: sqlbase.ColumnType_BIGINT, Width: 64},
nit: other cases have the width before the visible type, probably best to match them
pkg/sql/coltypes/arith.go, line 57 at r1 (raw file):
// TSerial represents a SERIAL type. type TSerial struct {
Why is this not just an alias for int now? And why is it a pointer to an int?
pkg/sql/sqlbase/structured.proto, line 153 at r1 (raw file):
enum VisibleType { NONE = 0; INTEGER = 1; // Deprecated, remove post-2.2.
post-2.2 might be the wrong wording here, we're going to keep it until 2.3?
Prior to this patch, CockroachDB maintained an unnecessary distinction between "INT" and "INTEGER", between "BIGINT" and "INT8", etc. This distinction is unnecessary but also costly, as we were paying the price of a "name" attribute in coltypes.TInt, with a string comparison and hash table lookup on every use of the type. What really matters is that the type shows up properly in introspection; this has already been ensured by various OID-to-pgcatalog mappings and the recently introduced `InformationSchemaTypeName()`. Any distinction beyond that is unnecessary and can be dropped from the implementation. Release note: None
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/table_test.go, line 58 at r1 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
nit: other cases have the width before the visible type, probably best to match them
Done.
pkg/sql/coltypes/arith.go, line 57 at r1 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
Why is this not just an alias for int now? And why is it a pointer to an int?
It's not an alias because it has separate handling in CREATE, etc. It has to be linked by pointer because the pointer itself is compared in other places to check whether it's equal to Int2/Int4/Int8 etc. But I suppose it can be embedded instead of named. Done.
pkg/sql/sqlbase/structured.proto, line 153 at r1 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
post-2.2 might be the wrong wording here, we're going to keep it until 2.3?
I meant to say remove it after 2.2 has been released. I'd have written "remove in 2.2" if I had meant that it needed to be removed before 2.2 was released. I don't know what version comes after that (2.3, 3.0, 30?) so I didn't want to put a more specific number.
BramGruneir
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status:
BramGruneir
left a comment
There was a problem hiding this comment.
Reviewable status:
|
TFYR! |
Supersedes #28690 for ease of review.
Prior to this patch, CockroachDB maintained an unnecessary distinction
between "INT" and "INTEGER", between "BIGINT" and "INT8", etc.
This distinction is unnecessary but also costly, as we were paying the
price of a "name" attribute in coltypes.TInt, with a string comparison
and hash table lookup on every use of the type.
What really matters is that the type shows up properly in
introspection; this has already been ensured by various
OID-to-pgcatalog mappings and the recently introduced
InformationSchemaTypeName().Any distinction beyond that is unnecessary and can be dropped from the
implementation.
Release note: None