This change is 

@yuzefovich can you review the change to Fingerprint?

I think @rohany made some recent changes here so that ArrayFamily would use ed.Encode method below although it "must be value encoded".

Nah, arrays have key encodings now.

yeah

Ok sql exec people. Here's a stumper. The Fingerprint change appears to have broken a few tests.

is now returning 0 rows only in the fakedist-vec-auto-disk configuration.

no longer produces an error but is reported as success in the fakedist-vec-auto-disk and fakedist-disk configurations. Any idea why that might happen? It is possible the Fingerprint change has exposed some places where there are some incorrect assumptions about uhh things.

For example. sqlbase.MustBeValueEncoded says that tuples must be value encoded:

and yet the above query for where uses the key encoding in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sqlbase/column_type_encoding.go#L148 due to some disk-backed hash join thing, and even though tuples don't have a corresponding decode method. Also, their key encode method is highly suspect. (I discovered this in a separate PR I was working on trying to make sense of MustBeValueEncoded, EncodeTableKey, and DecodeTableKey, none of which agree on what things must be key encoded, see https://github.com/mjibson/cockroach/tree/enc-tests for details. It surprisingly showed up here too, and so it looks like my investigation now needs to be completed instead of abandoned.)

Questions:

1. Why is the tuple key encoding used in the disk hash joiner if MustBeValueEncoded returns true for tuples?
2. Why does the tuple key encoding exist since it has no corresponding decode method?
3. Why did the two above tests change for certain configurations due to the Fingerprint change?

I think there is some slight overloading of how MustBeValueEncoded is used unfortunately. It seems to be mostly when we want to see if a type can be indexed, rather than "can we actually call encode table key on this type". We seem to have added key encodings for things that we would never actually index for use in various key encoding needs during execution (group by, order by). In these cases, we use the key encoding just as a hash (since some of this code was written before we had finger print) -- i think this answers 1 and 2. I don't know about three.

Ok so maybe...we can use Fingerprint in more places now? I'll audit uses of EncodeTableKey.

That sounds reasonable — encdatum.encode is probably useful to look at too. Most of the calls come from there.

I think I see the issue: in DiskRowContainer.encodeRow we use Encode (on the right side of the hash join), but when we're probing in hashDiskRowBucketIterator.Reset we're calling encodeEqualityCols which under the hood is using Fingerprint.

This diff fixes the issue with row containers:

diff --git a/pkg/sql/rowcontainer/hash_row_container.go b/pkg/sql/rowcontainer/hash_row_container.go
index 93fe65c1d2..b8ad6bfe95 100644
--- a/pkg/sql/rowcontainer/hash_row_container.go
+++ b/pkg/sql/rowcontainer/hash_row_container.go
@@ -121,7 +121,12 @@ func encodeColumnsOfRow(
                if row[colIdx].IsNull() && !encodeNull {
                        return nil, true, nil
                }
-               appendTo, err = row[colIdx].Fingerprint(colTypes[i], da, appendTo)
+               // Note: we cannot compare VALUE encodings because they contain column IDs
+               // which can vary.
+               // TODO(radu): we should figure out what encoding is readily available and
+               // use that (though it needs to be consistent across all rows). We could add
+               // functionality to compare VALUE encodings ignoring the column ID.
+               appendTo, err = row[colIdx].Encode(colTypes[i], da, sqlbase.DatumEncoding_ASCENDING_KEY, appendTo)
                if err != nil {
                        return appendTo, false, err
                }
@@ -422,7 +427,7 @@ func (i *hashMemRowIterator) computeKey() error {
        i.curKey = i.curKey[:0]
        for _, col := range i.storedEqCols {
                var err error
-               i.curKey, err = row[col].Fingerprint(i.types[col], &i.columnEncoder.datumAlloc, i.curKey)
+               i.curKey, err = row[col].Encode(i.types[col], &i.columnEncoder.datumAlloc, sqlbase.DatumEncoding_ASCENDING_KEY, i.curKey)
                if err != nil {
                        return err
                }

The explanation is that HashDiskRowContainer is implemented using DiskRowContainer with the equality columns (i.e. the columns to hash) of the former being the ordering columns for the latter, and those ordering columns are used to compute the keys of the rows (in encodeRow) so that we could store the row in the sorted order. This way we store the build (right) side of the join, but for the probe (left) side we use hashMemRowIterator to compute the key of the probing row. The key computation methods must be the same in both places, otherwise, the results of the join can be incorrect. #45229 broke this synchronization by changing the key computation method in hashMemRowIterator.computeKey to use Fingerprint. So we have to either use Fingerprint in encodeRow or use Encode in computeKey. The first choice doesn't seem to work because Fingerprint doesn't provide the ordering we need in DiskRowContainer, so we need to use the second approach. @rohany does this sound reasonable?

That seems correct, but we will then error out for types that we can't key encode. We can't know in advance whether we are going to spill to disk here either.

Why does the disk row container necessarily need ordering properties of the columns?

Why does the disk row container necessarily need ordering properties of the columns?

DiskRowContainer implements "hash row container" by sorting all rows on the ordering (i.e. hash) columns and using the ordering property to provide the "hashing" behavior (i.e. we would seek to the first row that has the same hash columns and then iterate from that row one row at a time forward until the hash columns remain the same). If we don't have the ordering property, then the necessary invariant that all rows that hash to the same value are contiguous is not maintained.

bors r+

Merge conflict (retrying...)

Build failed

• GitHub CI (Cockroach)

bors r+

Build failed (retrying...)

• GitHub CI (Cockroach)

Canceled

bors r+

Build failed

• GitHub CI (Cockroach)

bors r+

Build succeeded

• GitHub CI (Cockroach)