sql: use IndexFetchSpec in TableReader

[RaduBerinde]

row: replace fetcher args with IndexFetchSpec

This commit replaces the row fetcher table args with IndexFetchSpec.

Release note: None

sql: use IndexFetchSpec in TableReader

This commit removes the table descriptor from TableReader and replaces
it with an IndexFetchSpec.

Eventually, we will use the same IndexFetchSpec to form the columnar
batch directly in KV.

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @yuzefovich)

---

pkg/sql/catalog/descpb/index_fetch.proto, line 81 at r2 (raw file):

  // TableModificationTime is the timestamp of the transaction which last
  // modified the table descriptor.
  // TODO(radu, otan): take this into account during planning and remove it from

CC @otan

[yuzefovich]

Nice!

Reviewed 11 of 11 files at r1, 29 of 29 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @RaduBerinde)

---

pkg/ccl/changefeedccl/rowfetcher_cache.go, line 168 at r1 (raw file):

	// TODO(dan): Allow for decoding a subset of the columns.
	publicCols := tableDesc.PublicColumns()
	colIDs := make([]descpb.ColumnID, len(publicCols))

nit: are there many places where we need IDs of all public columns? It might be worth introducing a method on the table descriptor for it.

---

pkg/ccl/cliccl/debug_backup.go, line 566 at r1 (raw file):

	ctx context.Context, entry backupccl.BackupTableEntry, codec keys.SQLCodec,
) (row.Fetcher, error) {
	colIDs := make([]descpb.ColumnID, len(entry.Desc.PublicColumns()))

nit: looks like here we could reuse the new method.

---

pkg/sql/catalog/descpb/index_fetch.proto, line 72 at r2 (raw file):

  }

  optional uint32 version = 1 [(gogoproto.nullable) = false];

nit: can you add some words for why we need versioning in this spec, separate from the DistSQL versions?

---

pkg/sql/execinfrapb/flow_diagram.go, line 170 at r2 (raw file):

		for i := range keyDirs {
			keyDirs[i] = encoding.Ascending
			if tr.FetchSpec.KeyAndSuffixColumns[i].Direction == descpb.IndexDescriptor_ASC {

Shouldn't we use DESC in the conditional?

---

pkg/sql/row/fetcher_mvcc_test.go, line 86 at r1 (raw file):

	desc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, `d`, `parent`)
	var colIDs []descpb.ColumnID
	for _, col := range desc.PublicColumns() {

nit: ditto

---

pkg/sql/row/fetcher_test.go, line 55 at r1 (raw file):

		}
	} else {
		for _, col := range publicCols {

nit: ditto

[otan]

---

pkg/sql/catalog/descpb/index_fetch.proto, line 81 at r2 (raw file):

Previously, RaduBerinde wrote…

CC @otan

ack, sorry i haven't had time recently to take a deeper look.

[RaduBerinde]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @yuzefovich)

---

pkg/sql/execinfrapb/flow_diagram.go, line 170 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Shouldn't we use DESC in the conditional?

Done.

[yuzefovich]

Reviewed 35 of 35 files at r3, 54 of 54 files at r4, 8 of 8 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @RaduBerinde)

---

pkg/sql/execinfrapb/flow_diagram.go, line 147 at r5 (raw file):

func (tr *TableReaderSpec) summary() (string, []string) {
	details := make([]string, 0, 3)
	details = append(details, indexDetail(&tr.Table, tr.IndexIdx))

nit: it could have been cleaner to refactor indexDetail in a separate commit (to print out the actual name of the primary index rather than "primary"). Then there would be less changes in the main commit of this PR. I guess at this point it's probably not worth the effort. It might be a good idea to still make such a change.

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @yuzefovich)

---

pkg/sql/execinfrapb/flow_diagram.go, line 147 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: it could have been cleaner to refactor indexDetail in a separate commit (to print out the actual name of the primary index rather than "primary"). Then there would be less changes in the main commit of this PR. I guess at this point it's probably not worth the effort. It might be a good idea to still make such a change.

I'll try to do that and open a separate PR, then rebase this one.

[RaduBerinde]

---

pkg/sql/execinfrapb/flow_diagram.go, line 147 at r5 (raw file):

Previously, RaduBerinde wrote…

I'll try to do that and open a separate PR, then rebase this one.

#76406

[yuzefovich]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @RaduBerinde)

---

pkg/sql/execinfrapb/flow_diagram.go, line 147 at r5 (raw file):

Previously, RaduBerinde wrote…

#76406

Sounds good, thanks!

[RaduBerinde]

Rebased after merging the separate change to flow diagrams.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @jordanlewis, @RaduBerinde, and @yuzefovich)

[yuzefovich]

Reviewed 62 of 62 files at r6, 62 of 62 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @jordanlewis)

[RaduBerinde]

I made a small change to put the descriptor modification time directly in TableReaderSpec instead of IndexFetchSpec. This keeps the IndexFetchSpec smaller and will reduce the churn if we remove it later.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @jordanlewis, and @yuzefovich)

[RaduBerinde]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)