importccl: support DEFAULT columns for constant DEFAULT expressions

[Anzoteh96]

Currently, IMPORT does not support importing DEFAULT columns, and non-targeted columns must be nullable.

This PR adds support for IMPORT INTO that involves non-targeted columns with DEFAULT expressions that are constant. Constant expressions are those that return the same value in different statements. Some examples are:
-- Literals (booleans, strings, integers, decimals, dates)
-- Functions where each argument is a constant expression and the functions themselves depend solely on their arguments. Examples are arithmetic operations, boolean logical operations (AND, OR), string operations like concat and repeat.

In particular, functions like now() and nextval that return different values across different transactions are not supported.

This is done by evaluating the required default expressions in the function sql/row/row_converter:NewDatumRowConverter.

To illustrate this, given file.csv with content:

35,test
72,hello

this will be the expected behaviour of the following line:

> CREATE TABLE t (a INT DEFAULT 53, b STRING, c INT DEFAULT 42);
> IMPORT INTO t (a, b) CSV DATA ('nodelocal://1/file.csv');
> SELECT * FROM t;
 a |  b | c
----+----- + ------
  35 |  test | 42
  72 | hello | 42

Note that the default expression for (c) is used since it's non-targeted, but the default expression for (a) is ignored in favour of the column in the csv.

Fixes #48253

Release note (general change): IMPORT INTO now supports columns with constant default expressions, and non-targeted columns with constant default expressions are no longer required to be nullable.

[cockroach-teamcity]

This change is 

[miretskiy]

I think this pr should have release notes; it's a user visible change.

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

---

pkg/ccl/importccl/import_stmt.go, line 593 at r1 (raw file):

			// expressions are nullable.
			for _, col := range found.VisibleColumns() {
				if len(isTargetCol) != 0 && !isTargetCol[col.Name] && !col.IsNullable() && !col.HasDefault() {

can you wrap this long if statement?

---

pkg/ccl/importccl/import_stmt_test.go, line 2490 at r1 (raw file):

	})

	// Supporting import into with non-targetted default or nullable columns are not implemented yet at this point.

can you wrap long lines (100 char or so)?

---

pkg/sql/row/row_converter.go, line 270 at r1 (raw file):

	var txCtx transform.ExprTransformContext
	semaCtx := tree.MakeSemaContext()
	// We do not currently support DEFAULT expressions on target or non-target

does this comment need to be updated?

---

pkg/sql/row/row_converter.go, line 303 at r1 (raw file):

	c.Datums = make([]tree.Datum, len(targetColDescriptors), len(cols))

	// Check for a hidden column. This should be the unique_rowid PK if present.

this comment needs to be updated; we no longer just check hidden columns, but also assign defaults

[pbardea]

@miretskiy just a head's up that I think this PR is still a WIP (I noticed it was marked as draft).

@Anzoteh96 I sometimes update the title temporarily to be prefixed with [WIP] since it's not easy to tell if a PR is marked as draft in reviewable.

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

[Anzoteh96]

Just changed that to "Ready for review". Made small changes as per below (except adding Release Note).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt.go, line 593 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

can you wrap this long if statement?

Done.

---

pkg/ccl/importccl/import_stmt_test.go, line 2490 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

can you wrap long lines (100 char or so)?

Test removed: as per the discussion with @pbardea , we will focus only on adding support for IMPORT INTO non-targeted columns withDEFAULT expressions as it specifies exactly which columns in a table t should the columns in CSV map to.

Having non-targeted column with DEFAULT expression for IMPORT TABLE, on the other hand, could introduce ambiguity for the following reason: say we have a CSV file my_csv of types (INT, INT, INT) and we do the following:

IMPORT TABLE t (a INT, b INT DEFAULT 10, c INT, d INT DEFAULT 8) CSV DATA ("my_csv")

How to decide which column (b or d) to be assigned default value instead of taken from the CSV?

---

pkg/sql/row/row_converter.go, line 270 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

does this comment need to be updated?

Done.

---

pkg/sql/row/row_converter.go, line 303 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

this comment needs to be updated; we no longer just check hidden columns, but also assign defaults

Done.

[Anzoteh96]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt_test.go, line 2479 at r3 (raw file):

	// data from source file (like CSV) is used. It also checks IMPORT TABLE works
	// when there are default columns.
	t.Run("import-into-default", func(t *testing.T) {

Note: these are the tests I have now. Feel free to suggest anything else (including the specifications of this IMPORT).

[miretskiy]

Is this still a work in progress or is it ready for review?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @pbardea)

[Anzoteh96]

It's ready for review now :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @miretskiy, and @pbardea)

[pbardea]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Anzoteh96, @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt.go, line 590 at r3 (raw file):

			}

			// Ensure that non-target columns that doesn't have default

nit: s/doesn't/don't/

---

pkg/ccl/importccl/import_stmt.go, line 594 at r3 (raw file):

			for _, col := range found.VisibleColumns() {
				if len(isTargetCol) != 0 && !isTargetCol[col.Name] &&
					!col.IsNullable() && !col.HasDefault() {

This may be a personal thing, but I find this logic a lot easier to understand as !(col.IsNullable() || col.HasDefault()) since then we're just enforcing that every non-target column is either nullable or has a default value. Again, it may just be personal preference but something to consider.

---

pkg/ccl/importccl/import_stmt.go, line 595 at r3 (raw file):

				if len(isTargetCol) != 0 && !isTargetCol[col.Name] &&
					!col.IsNullable() && !col.HasDefault() {
					return errors.Errorf("all non-target columns in IMPORT INTO must be nullable")

I think we need to update this error message adding the fact that these columns must be nullable or have a default. While we're here I think we can s/Errorf/Error/ since this error message isn't using any formatting directives.

---

pkg/ccl/importccl/import_stmt_test.go, line 2475 at r3 (raw file):

	})

	// Test that IMPORT INTO works when columns with default expression is present.

nit: s/default expression is/default expressions are/

---

pkg/ccl/importccl/import_stmt_test.go, line 2479 at r3 (raw file):

Previously, Anzoteh96 (Anzo Teh) wrote…

Note: these are the tests I have now. Feel free to suggest anything else (including the specifications of this IMPORT).

I would be interested in adding a test case with a default column that also has a NOT NULL constraint given that we have some checks around nullability and default columns. I'd also be interested in seeing some tests cases where the data has NULL values that are inserted into default columns. I would also be interested in a test case with a more complex default expressions, perhaps using some builtins?

---

pkg/ccl/importccl/import_stmt_test.go, line 2495 at r3 (raw file):

		})
		t.Run("is-target", func(t *testing.T) {
			data = "1,36\n2,42"

I think having the last value set to the same values as the default expression is a bit confusing in this case - I'm not sure what we're testing here? From a test perspective, I think it would be more clear if this value was set to something else to clearly indicate that we don't expect the default expression to be evaluated.

---

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

	// Check for a hidden column. This should be the unique_rowid PK if present.
	// In addition, check for non-targeted columns with non-null DEFAULT expressions.
	colNameSet := make(map[string]struct{}, len(targetColDescriptors))

I think targetColNames or something of that sort may be a more descriptive name.

---

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

				datum, err := defaultExprs[i].Eval(evalCtx)
				if err != nil {
					return nil, errors.New("Error parsing default expression")

nit: we usually format error messages with a lower-case first letter

[miretskiy]

Reviewed 2 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Anzoteh96, @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt.go, line 595 at r3 (raw file):

				if len(isTargetCol) != 0 && !isTargetCol[col.Name] &&
					!col.IsNullable() && !col.HasDefault() {
					return errors.Errorf("all non-target columns in IMPORT INTO must be nullable")

let's update this error message: "all non-target columsn ... must be nullable or have default expression set"

---

pkg/ccl/importccl/import_stmt_test.go, line 2523 at r3 (raw file):

				{"11", "string3", "33"},
				{"29", "string4", "33"}})
		})

See the comment I left in row converter.
You definitely need a good test case where default expression changes.
For example:

create sequence testseq;
create table a(a int primary key default unique_rowid(), b string, c int default nextval('testseq'));

If you were to import INTO column B, a CSV file with the following data:
un
deux
trois

You expect something like this to be stored in the table.
a | b | c
---------------------+-------+----
566184438199091201 | un | 1
566184438203088897 | deux | 2
566184438203678721 | trois | 3

---

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

			c.Datums = append(c.Datums, nil)
		} else {
			if _, ok := colNameSet[col.Name]; !ok && col.DefaultExpr != nil {

I know "ok" is pretty common pattern, but consider renaming to isTargetCol (or similar).

---

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

					return nil, errors.New("Error parsing default expression")
				}
				c.Datums = append(c.Datums, datum)

This looks a bit suspicious to me. Disclaimer: I'm not an sql expert; so perhaps somebody else should also take a look at this (@dt)?

Reading documentation on "DEFAULT" expressions:
"The value may be either a hard-coded literal or an expression that is evaluated at the time the row is created."

So, the problem here is that datum converter is created once (or few times, if using parallel importer), and is then reused for the duration of the import. That means that this expression would not actually be evaluated at row creating time, but instead at the time when we start the import.

That seems a bit wrong to me. I think this code ought to be moved to Row() function below, and handled similarly to how we handle primary key (note, above, we append nil in the case of Hidden column because we want to evaluate the primary key default expression, namely unique_rowid(), when we actually call Row()).

[miretskiy]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Anzoteh96, @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt.go, line 594 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

This may be a personal thing, but I find this logic a lot easier to understand as !(col.IsNullable() || col.HasDefault()) since then we're just enforcing that every non-target column is either nullable or has a default value. Again, it may just be personal preference but something to consider.

This is not just you -- I also find it harder to read..
There are few things we can do to help things out:

Before this loop, I would add:

isTargetCol := func(name string) bool {
return len(isTargetCol) > 0 && isTargetCol[col.Name]
}

Then, in this loop, this condition becomes
if !(isTargetCol(col.Name) || col.IsNullable() || col.HasDefault()) {
}

[Anzoteh96]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Anzoteh96, @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt.go, line 590 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: s/doesn't/don't/

Done.

---

pkg/ccl/importccl/import_stmt.go, line 594 at r3 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

This is not just you -- I also find it harder to read..
There are few things we can do to help things out:

Before this loop, I would add:

isTargetCol := func(name string) bool {
return len(isTargetCol) > 0 && isTargetCol[col.Name]
}

Then, in this loop, this condition becomes
if !(isTargetCol(col.Name) || col.IsNullable() || col.HasDefault()) {
}

Did an alternative change: move len(isTargetCol) is an outermost loop (as this is a value that stays the same across all the columns), so that we can have !(isTarget[Col.Name] || col.IsNullable() || col.HasDefault()).

---

pkg/ccl/importccl/import_stmt.go, line 595 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I think we need to update this error message adding the fact that these columns must be nullable or have a default. While we're here I think we can s/Errorf/Error/ since this error message isn't using any formatting directives.

error.Error seemed to give some sort of "missing implementation" error, I changed it to errors.New(...).

---

pkg/ccl/importccl/import_stmt.go, line 595 at r3 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

let's update this error message: "all non-target columsn ... must be nullable or have default expression set"

Done.

---

pkg/ccl/importccl/import_stmt_test.go, line 2475 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: s/default expression is/default expressions are/

Done.

---

pkg/ccl/importccl/import_stmt_test.go, line 2495 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I think having the last value set to the same values as the default expression is a bit confusing in this case - I'm not sure what we're testing here? From a test perspective, I think it would be more clear if this value was set to something else to clearly indicate that we don't expect the default expression to be evaluated.

Done. (Changed to 37).

---

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

Previously, pbardea (Paul Bardea) wrote…

nit: we usually format error messages with a lower-case first letter

Done.

---

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

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

This looks a bit suspicious to me. Disclaimer: I'm not an sql expert; so perhaps somebody else should also take a look at this (@dt)?

Reading documentation on "DEFAULT" expressions:
"The value may be either a hard-coded literal or an expression that is evaluated at the time the row is created."

So, the problem here is that datum converter is created once (or few times, if using parallel importer), and is then reused for the duration of the import. That means that this expression would not actually be evaluated at row creating time, but instead at the time when we start the import.

That seems a bit wrong to me. I think this code ought to be moved to Row() function below, and handled similarly to how we handle primary key (note, above, we append nil in the case of Hidden column because we want to evaluate the primary key default expression, namely unique_rowid(), when we actually call Row()).

You're right: unfortunately, the current implementation only supports DEFAULT literal expressions for non-targeted columns. For default values that are functions (that are sequence-related) the change is more elaborate than what you outlined: it also involves changing the Sequence field in the evalCtx into to either a planner or importSequenceOperators field (and in the later case, we also need to implement func ( *importSequenceOperators) IncrementSequence).

Apologies for not catching that earlier: I will post an update when this is resolved.

[miretskiy]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Anzoteh96, @dt, @miretskiy, and @pbardea)

---

pkg/ccl/importccl/import_stmt.go, line 595 at r3 (raw file):

Previously, Anzoteh96 (Anzo Teh) wrote…

error.Error seemed to give some sort of "missing implementation" error, I changed it to errors.New(...).

You might use errors.Newf(...) if you also want to include column name in the error message.