sqlbase: add the missing comments and group functions by purpose #28943

knz · 2018-08-22T08:20:45Z

First commits from #28942 and priors.
Forked from #28690.

The file table.go was getting unwieldy and was containing functions
from different area of concern. This was making maintenance
particularly burdensome.

This patch splits the functionality in the following files:

index_encoding.go: index key prefixes, differences between primary
and secondary encodings, how to organize columns in an index.
column_type_encoding.go: encoding of SQL values into bytes
towards KV.
column_type_properties.go: conversions between various
representations of types in the SQL layer; derivation of properties
from SQL types.
datum_alloc.go: the DatumAlloc helper.

Release note: None

cockroach-teamcity · 2018-08-22T08:20:53Z

This change is

knz · 2018-08-22T08:23:23Z

Note: I was careful to ensure there is no functionality change.

BramGruneir

With some requested changes.

I love the code reorganizing and adding comments to it.

However, next time, please split this into multiple commits:

1 commit to move the code
1 commit to add the comments

It makes the reviewers life a lot easier to skip the code movement (besides where it lands) and concentrate on the updated descriptions.

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 18 of 18 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/distsqlplan/physical_plan.go, line 900 at r3 (raw file):

// another, excluding its VisibleType type alias, which doesn't effect
// equality.
func equivalentTypes(c, other *sqlbase.ColumnType) bool {

This seems out of place in this file. Unless this is the only place where it is used.

pkg/sql/sqlbase/column_type_encoding.go, line 930 at r3 (raw file):

}

func numBytesInBitArray(numBits int) int {

this could use a comment

pkg/sql/sqlbase/column_type_encoding.go, line 1031 at r3 (raw file):

}

func encodeArrayElement(b []byte, d tree.Datum) ([]byte, error) {

could use a comment here too while you're at it

pkg/sql/sqlbase/column_type_properties.go, line 48 at r3 (raw file):

// Some notional complexity arises from the fact there are fewer
// different types.T than different coltypes.T/ColumnTypes. This is
// because some distinctions important at the time of data persistence

some distinctions which are important at the

pkg/sql/sqlbase/column_type_properties.go, line 51 at r3 (raw file):

// (or casts) are not useful to keep for in-flight values; for example
// the final required precision for DECIMAL values.
//

nit: extra blank comment lines

pkg/sql/sqlbase/column_type_properties.go, line 129 at r3 (raw file):

		base.Width = int32(t.Scale)
		base.Precision = int32(t.Prec)

nit: remove this extra line, and add one after the switch to match the earlier blanks. Or just remove all blank lines, since the style changed again later.

pkg/sql/sqlbase/column_type_properties.go, line 255 at r3 (raw file):

// type is not a character or bit string, or if the string's length is not bounded.
//
// This is used to populate information_schema.columns.character_maximum_length.

Generally, it's advisable not to declare in a function where it is being used. Since that is likely to rot and doesn't provide much extra info.

For example, if we have a new use for it, should we add it here? If we remove that primary use of it, this comment also has to be changed.

(This comment applies to this whole file)

pkg/sql/sqlbase/column_type_properties.go, line 266 at r3 (raw file):

}

// MaxOctetLength returns the maximum the maximum possible length in octets of a

the maximum is repeated

pkg/sql/sqlbase/column_type_properties.go, line 313 at r3 (raw file):

}

// NumericPrecisionRadix returns implicit precision radix of numeric

returns the implicit

pkg/sql/sqlbase/column_type_properties.go, line 349 at r3 (raw file):

//
// This function is for internal use in this package and only exported
// for testing in pgwire. Avoid using directly!

If this is only used for testing, we should move it to column_type_properties_test.go to ensure it is not used in any main codepath.

pkg/sql/sqlbase/column_type_properties.go, line 409 at r3 (raw file):

// columnSemanticTypeToDatumType determines a types.T that can be used
// to instantiate in-memory representation of values of the given

instantiate an in-memory representation of values for the given

pkg/sql/sqlbase/column_type_properties.go, line 463 at r3 (raw file):

// ToDatumType converts the ColumnType to a types.T suitable to instantiate
// in-memory representations of the values of a column of the given ColumnType.

This sentence is confusing and needs work.

pkg/sql/sqlbase/column_type_properties.go, line 496 at r3 (raw file):

// CheckValueWidth checks that the width (for strings, byte arrays, and
// bit string) and scale (for decimals) of the value fits the specified

bit strings) and

pkg/sql/sqlbase/index_encoding.go, line 58 at r3 (raw file):

// /<parent_table_id>/<parent_index_id>/<field_1>/<field_2>/NullDesc/<table_id>/<index_id>/<field_3>/<family>
//
// Returns the key and whether any of the encoded values were NULLs.

This comment should be with the first sentence, since it describes the function.

pkg/sql/sqlbase/index_encoding.go, line 246 at r3 (raw file):

}

func appendEncDatumsToKey(

Could use a comment.

pkg/sql/sqlbase/index_encoding.go, line 272 at r3 (raw file):

// index.ExtraColumnIDs respectively.
//
// MakeKeyFromEncDatums (see above) does not allow you to create a key

remove the (see above) and add Note: or Note that at the start

pkg/sql/sqlbase/index_encoding.go, line 319 at r3 (raw file):

// EncodeDatumKeyAscending encodes a datum using an order-preserving
// encoding.
// The encoding is lossy -- some datums need composite encoding where the

nit: the -- is strange. Perhaps a semicolon?

Similar for the datums version as well.

pkg/sql/sqlbase/index_encoding.go, line 666 at r3 (raw file):

// EncodeInvertedIndexKeys creates a list of inverted index keys
// by concatenating keyPrefix with the encodings of the column in the index.
//

Nit: remove space here. What this returns should be part of the main description.

pkg/sql/sqlbase/index_encoding.go, line 690 at r3 (raw file):

}

// EncodeInvertedIndexTableKeys encodes the paths in a JSON `val` and concatenates it with `inKey`and returns

lint: This comment seems like it is longer than our normal max.

pkg/sql/sqlbase/index_encoding.go, line 838 at r3 (raw file):

//  - int (converts to DInt)
//  - string (converts to DString)
func TestingMakePrimaryIndexKey(desc *TableDescriptor, vals ...interface{}) (roachpb.Key, error) {

This should be declared in index_encoding_test.go so it can never be used in non-test code.

pkg/sql/sqlbase/index_encoding.go, line 890 at r3 (raw file):

// belongs to a table where its equivalence signature and all its interleave
// ancestors' signatures can be found in validEquivSignatures.
// validEquivSignatures: a map containing equivalence signatures of valid

remove the colon and turn this into a sentence, the colon just makes it look like you'll have a list of explanations but there is only one.

pkg/sql/sqlbase/index_encoding.go, line 1039 at r3 (raw file):

//    /table/index/<parent-pk1>/.../<parent-pkX>
//
// We return the maximum number of <tokens> in this prefix.

We -> This

These will be used to break up table.go in smaller pieces. Release note: None

Release note: None

…estutils Release note: None

Release note: None

knz

Thanks Bram for pushing me for the extra mile. I have split the commits to distinguish the movement and the documentation, as you recommended.

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/distsqlplan/physical_plan.go, line 900 at r3 (raw file):