pkg: Add support for user specified target cols in IMPORT INTO

[adityamaru27]

This change adds the ability for users to specify a subset of the columns of an existing table to import into, from a CSV data source.
(Part of the larger roadmap #26834)

[cockroach-teamcity]

This change is 

[dt]

Reviewed 4 of 4 files at r1, 11 of 12 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru27 and @dt)

---

pkg/ccl/importccl/import_stmt.go, line 440 at r2 (raw file):

				intoCols = append(intoCols, name.String())
			}
			tableDetails = []jobspb.ImportDetails_Table{{Desc: &importing, IsNew: false, TargetCols: intoCols}}

should we be checking that the named cols are actually valid here?

I think we also want to validate that every column is not targetted has a default if we support filling defaults or has an implicit null default and is nullable, otherwise there is no point in starting the import, right?

---

pkg/ccl/importccl/import_stmt.go, line 631 at r2 (raw file):

	parentID sqlbase.ID,
	tables map[string]*sqlbase.TableDescriptor,
	targetCols map[string]*distsqlpb.ReadImportDataSpec_TargetColList,

minor nit here/throughout: as a reader, I had a little trouble with this type and knowing what it was/how to read it at first glance. I think I managed to figure it out but the parts I struggled with were what are the keys (string could be anything) and then, is it the same as the tables map right above it? if so, should we just be changing that to contain structs? if not, when/why is it different? also, if *distsqlpb.ReadImportDataSpec_TargetColList is just a long-winded []string maybe we could use that for internal calls and just wrap it in the proto message when building the spec proto.

Anyway, this is all just personal preference / style opinion, and I realize you've spent more time in here figuring what you need to plumb though than I have, so I'm fine with it as-is if you are.

---

pkg/sql/row/row_converter.go, line 231 at r3 (raw file):

	var txCtx transform.ExprTransformContext
	// Although we don't yet support DEFAULT expressions on visible columns,

can you update this comment to clarify how targeting fewer columns works w.r.t. defaults?

I think I gather that we don't support DEFAULT yet (we should for non-targeted cols) but what does that mean?

[adityamaru27]

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

---

pkg/ccl/importccl/import_stmt.go, line 440 at r2 (raw file):

Previously, dt (David Taylor) wrote…

should we be checking that the named cols are actually valid here?

I think we also want to validate that every column is not targetted has a default if we support filling defaults or has an implicit null default and is nullable, otherwise there is no point in starting the import, right?

Done. Added a validity check for named cols, also added a check to disallow DEFAULT expressions and non-nullable non-target columns.

---

pkg/ccl/importccl/import_stmt.go, line 631 at r2 (raw file):

Previously, dt (David Taylor) wrote…

minor nit here/throughout: as a reader, I had a little trouble with this type and knowing what it was/how to read it at first glance. I think I managed to figure it out but the parts I struggled with were what are the keys (string could be anything) and then, is it the same as the tables map right above it? if so, should we just be changing that to contain structs? if not, when/why is it different? also, if *distsqlpb.ReadImportDataSpec_TargetColList is just a long-winded []string maybe we could use that for internal calls and just wrap it in the proto message when building the spec proto.

Anyway, this is all just personal preference / style opinion, and I realize you've spent more time in here figuring what you need to plumb though than I have, so I'm fine with it as-is if you are.

Thanks for this suggestion, the map tables and targetCols did have a 1:1 relation so I just made tables have structs instead. This allowed us to avoid plumbing the TargetColList separately and made it cleaner.

---

pkg/sql/row/row_converter.go, line 231 at r3 (raw file):

Previously, dt (David Taylor) wrote…

can you update this comment to clarify how targeting fewer columns works w.r.t. defaults?

I think I gather that we don't support DEFAULT yet (we should for non-targeted cols) but what does that mean?

Done.

[adityamaru27]

@dt RFAL. Addressed comments and added some more tests to ensure we do not support DEFAULT expression, or non-nullable non-target columns.

[adityamaru27]

TFTR!
bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)