sql: remove the interleaved logic from fetchers #70239

yuzefovich · 2021-09-15T02:39:48Z

row: simplify fetcher to only handle a single table

Previously, row.Fetcher could handle multiple tables at the same time.
This was needed to support the interleaved tables efficiently, but since
the interleaved tables are no more, we can simplify the fetcher now.

There are more things that can be removed from the fetcher about
handling of the interleaved tables, but I chose to defer them to a later
commit. This commit's main focus is simplifying the fetcher to work
under the assumption that it will be fetching only from a single table.

Addresses: #69150

Release note: None

colbuilder: remove the check for index join on the interleaved index

Interleaved indexes are no more.

Release note: None

sql: remove the interleaved logic from fetchers

This commit removes the remaining bits of handling the interleaved
tables from both row fetcher and cFetcher. It also removes a test that
verifies that we can read from the interleaved table restored from
a backup.

Release note: None

cockroach-teamcity · 2021-09-15T02:39:54Z

This change is

yuzefovich · 2021-09-15T21:23:01Z

Will hold off on merging this until 21.2.0 is out.

michae2

The best type of PR. 🙂

Searching for "interleave" in fetcher.go turned up a few more spots. Not sure how comprehensive you're trying to make this... if I'm asking too much just skip them.

Reviewed 6 of 9 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

pkg/sql/row/fetcher.go, line 147 at r1 (raw file):

Quoted 5 lines of code…

	// The spans of keys to return for the given table. Fetcher
	// ignores keys outside these spans.
	// This is irrelevant if Fetcher is initialize with only one
	// table.
	Spans            roachpb.Spans

Should this also be deleted? (Or the comment changed?)

pkg/sql/row/fetcher.go, line 188 at r1 (raw file):

// Fetcher handles fetching kvs and forming table rows for an
// arbitrary number of tables.

nit: Should this now say "a single table"?

pkg/sql/row/fetcher.go, line 222 at r1 (raw file):

Quoted 5 lines of code…

	// True if the index key must be decoded.
	// If there is more than one table, the index key must always be decoded.
	// This is only false if there are no needed columns and the (single)
	// table has no interleave children.
	mustDecodeIndexKey bool

Is this still necessary?

pkg/sql/row/fetcher.go, line 414 at r1 (raw file):

Quoted 10 lines of code…

	// - If there is more than one table, we have to decode the index key to
	//   figure out which table the row belongs to.
	// - If there are interleaves, we need to read the index key in order to
	//   determine whether this row is actually part of the index we're scanning.
	// - If there are needed columns from the index key, we need to read it.
	//
	// Otherwise, we can completely avoid decoding the index key.
	if !rf.mustDecodeIndexKey && (neededIndexCols > 0 || table.index.NumInterleavedBy() > 0 || table.index.NumInterleaveAncestors() > 0) {
		rf.mustDecodeIndexKey = true
	}

I think some of this can also be deleted.

pkg/sql/row/fetcher.go, line 713 at r1 (raw file):

Quoted 5 lines of code…

			if _, foundSentinel := encoding.DecodeIfInterleavedSentinel(keySuffix); foundSentinel {
				// We found an interleaved sentinel, which means that the key we just
				// found belongs to a different interleave. That means we have to go
				// through with index key decoding.
				unchangedPrefix = false

Should some of this also be deleted?

pkg/sql/row/fetcher.go, line 731 at r1 (raw file):

Quoted 6 lines of code…

			if !moreKVs {
				// The key did not match any of the table
				// descriptors, which means it's interleaved
				// data from some other table or index.
				continue
			}

Can this be deleted?

pkg/sql/row/fetcher.go, line 1562 at r1 (raw file):

Quoted 41 lines of code…

	for i := 0; i < index.NumInterleaveAncestors(); i++ {
		ancestor := index.GetInterleaveAncestor(i)
		length := int(ancestor.SharedPrefixLen)
		// Skip up to length values.
		for j := 0; j < length; j++ {
			if consumedCols == nCols {
				// We're done early, in the middle of an interleave.
				return origKeyLen - len(key), nil
			}
			l, err := encoding.PeekLength(key)
			if err != nil {
				return 0, err
			}
			key = key[l:]
			consumedCols++
		}
		var ok bool
		key, ok = encoding.DecodeIfInterleavedSentinel(key)
		if !ok {
			return 0, errors.New("unexpected lack of sentinel key")
		}
		// Skip the TableID/IndexID pair for each ancestor except for the
		// first, which has already been skipped in our input.
		for j := 0; j < 2; j++ {
			idLen, err := encoding.PeekLength(key)
			if err != nil {
				return 0, err
			}
			key = key[idLen:]
		}
	}
	// Decode the remaining values in the key, in the final interleave.
	for ; consumedCols < nCols; consumedCols++ {
		l, err := encoding.PeekLength(key)
		if err != nil {
			return 0, err
		}
		key = key[l:]
	}
	return origKeyLen - len(key), nil
}

Some of this can probably change.

yuzefovich

Oh yes, it's quite a pleasant exercise :)

Thanks for the review! I consciously deferred the removal of some other interleaved business from these files to make this commit simpler and serve the goal of making fetcher work only on one table. Updated the commit message to include this context.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2)

pkg/sql/row/fetcher.go, line 147 at r1 (raw file):