sql: use IndexFetchSpec in TableReader by RaduBerinde · Pull Request #76394 · cockroachdb/cockroach

RaduBerinde · 2022-02-10T20:00:57Z

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 · 2022-02-10T20:01:05Z

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 · 2022-02-10T21:23:03Z

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 · 2022-02-10T23:19:01Z

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)

This commit replaces the row fetcher table args with IndexFetchSpec. Release note: None

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)

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

RaduBerinde · 2022-02-11T23:48:59Z

bors r+

craig · 2022-02-12T01:49:38Z

Build succeeded:

GitHub CI (Cockroach)

RaduBerinde requested review from jordanlewis and yuzefovich February 10, 2022 20:00

RaduBerinde requested review from a team as code owners February 10, 2022 20:00

RaduBerinde requested review from a team and ajwerner and removed request for a team February 10, 2022 20:00

RaduBerinde commented Feb 10, 2022

View reviewed changes

yuzefovich reviewed Feb 10, 2022

View reviewed changes

RaduBerinde force-pushed the fetch-spec-tablereader branch from b955f0d to 3c9c8a7 Compare February 10, 2022 22:03

RaduBerinde requested a review from a team as a code owner February 10, 2022 22:03

RaduBerinde force-pushed the fetch-spec-tablereader branch from 3c9c8a7 to b99e59d Compare February 10, 2022 22:17

RaduBerinde commented Feb 10, 2022

View reviewed changes

yuzefovich approved these changes Feb 10, 2022

View reviewed changes

RaduBerinde commented Feb 10, 2022

View reviewed changes

yuzefovich approved these changes Feb 11, 2022

View reviewed changes

RaduBerinde force-pushed the fetch-spec-tablereader branch from b99e59d to 095e8f3 Compare February 11, 2022 16:09

RaduBerinde commented Feb 11, 2022

View reviewed changes

yuzefovich approved these changes Feb 11, 2022

View reviewed changes

row: replace fetcher args with IndexFetchSpec

2628f64

This commit replaces the row fetcher table args with IndexFetchSpec. Release note: None

RaduBerinde force-pushed the fetch-spec-tablereader branch from 095e8f3 to 533e23b Compare February 11, 2022 19:22

RaduBerinde commented Feb 11, 2022

View reviewed changes

sql: use IndexFetchSpec in TableReader

8ba916b

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

RaduBerinde force-pushed the fetch-spec-tablereader branch from 533e23b to 8ba916b Compare February 11, 2022 20:40

craig bot merged commit 0aacc53 into cockroachdb:master Feb 12, 2022

RaduBerinde deleted the fetch-spec-tablereader branch February 16, 2022 15:31

ajwerner mentioned this pull request Jun 1, 2022

sql,kv,storage: push column batch generation into kvserver #82323

Open

10 tasks

Conversation

RaduBerinde commented Feb 10, 2022

row: replace fetcher args with IndexFetchSpec

sql: use IndexFetchSpec in TableReader

Uh oh!

cockroach-teamcity commented Feb 10, 2022

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

otan commented Feb 10, 2022

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Feb 10, 2022

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Feb 11, 2022

Uh oh!

craig bot commented Feb 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants