feat(spanner/spannertest): Support generated columns #4742

philwitty · 2021-09-10T12:42:13Z

No description provided.

olavloite

Thanks for this change, I think it is a good step in the right direction, but there are a couple of things that need some looking into:

I get two test failures when I run the tests locally. One seems to simply be a leftover t.Fail() call in one of your tests. The other is in an existing test.
I think it would be better to try to integrate the check which columns a generated column depends on with the existing expression evaluation. This reduces the chances of having differences between the two.

spanner/spannertest/README.md

olavloite · 2021-09-13T07:20:41Z

spanner/spannertest/db.go

+						break
+					}
+				}
+				// We skip this column if any of its dependencies are null


Why do we need to do that?

olavloite · 2021-09-13T07:20:55Z

spanner/spannertest/db.go

+			row:  row,
+		}
+
+		// TODO: We would need to do a topoligical sort on dependencies to ensure we can


nit:

Suggested change

// TODO: We would need to do a topoligical sort on dependencies to ensure we can

// TODO: We would need to do a topological sort on dependencies to ensure we can

olavloite · 2021-09-13T07:22:50Z

spanner/spannertest/db.go

@@ -634,6 +683,10 @@ func (t *table) addColumn(cd spansql.ColumnDef, newTable bool) *status.Status {
 			// TODO: what happens in this case?
 			return status.Newf(codes.Unimplemented, "can't add NOT NULL columns to non-empty tables yet")
 		}
+		if cd.Generated != nil {


nit: This check should be extended to check whether the table is empty or not. Adding a generated column to an empty tables should be safe in this case.

This PR should also add a check to dropColumn to prevent a column that is a dependent of a generated column from being dropped.

Good point, thanks

This is already checking the table is non empty, as for checking if its a dependency of a generated column do you still think its worth adding that as we talked about dropping all dependency calculations above?

Sorry, I missed that it was already checking it.

As for the dependency checking:

I don't think you need any dependency checking here.

We should preferably use the dependency checking in dropColumn to prevent columns that are used by a generated column from being dropped. But preferably that dependency checking can be done using the method that also does the evaluation, so that we don't have to duplicate that code. Would it for example be possible to use default values for the evaluation context when we only want to check the dependencies?

I think that definitely sounds like the right approach, having a way to run the evaluator in with default values in some "complete" way (i.e. we'd have to check in an OR what is required to evaluate the RHS even if the LHS evaluates to true). It feels out of scope for me right now, given it will be a fair bit of extra work to get going well and I'm not overly confident with the evaluation code and typing. For now I'll just come up with the solution for adding columns as any other features should be able to be built on top without changes so doesn't feel wasteful

olavloite · 2021-09-13T07:36:16Z

spanner/spannertest/db.go

@@ -1090,3 +1147,40 @@ func parseAsDate(s string) (civil.Date, error) { return civil.ParseDate(s) }
 func parseAsTimestamp(s string) (time.Time, error) {
 	return time.Parse("2006-01-02T15:04:05.999999999Z", s)
 }
+
+// getIDsForGeneratedExpression returns a list of column names the expression depends on
+func getIDsForGeneratedExpression(e spansql.Expr) []spansql.ID {


I'm not sure this is the best way to do this from a maintenance and completeness perspective. Instead I think it might be better to edit the evalContext.evalExpr function to also return the id's that the expression needs to be evaluated. That would keep the logic for what is needed for a certain expression in one place, and reduce the chance of missing something, and also ensure that whenever we add support for new expressions, those are also automatically included in this array.
This implementation currently does not detect any dependencies on other columns if they are used in:

Function calls

Array expressions

Yeah I really didn't like this approach, it felt like it was prone to error and future failure but couldn't figure out something smarter. I think your suggestion sounds solid I can check into that, So adding a 3rd return argument to evalExpr, potentially returning and error and the list of dependencies?

Yes, something like that was my thought as well.

olavloite · 2021-09-13T07:42:25Z

spanner/spannertest/db_test.go

+	rows := slurp(t, iter)
+	// 2 * A * B = 2 * 3 * 3
+	if rows[0][1].(int64) != 18 {
+		t.Fatal("Generated value for C should have been 18")


nit: use got/want format, so something like Generated value for C mismatch\n Got: %v\nWant: %v

olavloite · 2021-09-13T07:44:18Z

spanner/spannertest/db_test.go

+		t.Fatal("Generated value for C should be nil")
+	}
+
+	t.Fail()


I don't think this belongs here.

olavloite · 2021-09-13T07:44:56Z

spanner/spannertest/db_test.go

+	if rows[0][1] != nil {
+		t.Fatal("Generated value for C should be nil")
+	}
+


Could we also add tests for update and delete?

olavloite · 2021-09-13T07:45:35Z

spanner/spannertest/funcs.go

@@ -52,6 +52,14 @@ var functions = map[string]function{
 			return strings.HasPrefix(s, prefix), spansql.Type{Base: spansql.Bool}, nil
 		},
 	},
+	"LOWER": {


I don't think this change belongs in this PR.

Can move it to a separate one, just smuggled it in here as I was testing with it

Ah, ok. I would prefer to have it in a separate PR, unless it is actually used in one of the test cases (I don't think it is at this moment, right?)

olavloite · 2021-09-13T07:49:29Z

spanner/spannertest/db_test.go

@@ -361,6 +362,83 @@ func TestConcurrentReadInsert(t *testing.T) {
 	}
 }

+func TestGeneratedColumn(t *testing.T) {


Could we also add tests for:

Adding a generated column to an existing table.

Altering a generated column.

Dropping a column that is a dependant of a generated column.

philwitty · 2021-09-13T08:28:11Z

Thanks for this change, I think it is a good step in the right direction, but there are a couple of things that need some looking into:

I get two test failures when I run the tests locally. One seems to simply be a leftover t.Fail() call in one of your tests. The other is in an existing test.

I think it would be better to try to integrate the check which columns a generated column depends on with the existing expression evaluation. This reduces the chances of having differences between the two.

Thanks for the quick review, I think that suggestion is great, I was hoping someone could offer a better suggestion!

philwitty · 2021-09-14T12:35:38Z

@olavloite I did a rewrite dropping all the column dependency stuff, pretty happy with it now, I don't see why that can't be added on top later when we want to support dropping columns that may be dependencies or resolving co-dependent generated columns in a reliable way. It felt like a tricky problem to solve though, needing default values and ensuring complete execution etc, a good rewrite of the db_eval stuff.

olavloite · 2021-09-14T13:17:23Z

spanner/spannertest/db.go

+			if col.Generated != nil {
+				res, err := ec.evalExpr(col.Generated)
+				if err != nil {
+					// We assume that if the expression ended up with a type error a


I looked into the evaluation of the different expressions, and the problem that you are running into is that those evaluations do not take into account that some of the input parameters could be NULL. So I think that this is a reasonable workaround for this for now, but we should probably try to fix the evaluation of expressions that may return null values and then remove this.

Yeah exactly, its very much a best effort but dumb approach. Cheers

google-cla · 2021-09-14T13:18:42Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

olavloite · 2021-09-14T13:19:16Z

@googlebot I consent.

olavloite

LGTM

olavloite · 2021-09-15T06:51:20Z

@psytale The build error is caused by the fact that the implementation uses [Errors.Is](https://pkg.go.dev/errors#Is), which is only supported from Go 1.13 and further. The Spanner client library supports Go 1.11 and onwards, so we need to change that bit as well. There are two files in the repository (errors112.go and errors113.go) that are conditionally added to the build depending on the Go version. You could add the method that you need there.

Feel free to let me know if you want me to look into it for you.

* No longer returning code.InvalidArgument when calling db_eval.evalFunc but that seems consistent with the rest of the module, no tests failed so I assume its not behaviour relied upon, we could check for the error type instead

philwitty · 2021-09-15T08:23:30Z

@psytale The build error is caused by the fact that the implementation uses [Errors.Is](https://pkg.go.dev/errors#Is), which is only supported from Go 1.13 and further. The Spanner client library supports Go 1.11 and onwards, so we need to change that bit as well. There are two files in the repository (errors112.go and errors113.go) that are conditionally added to the build depending on the Go version. You could add the method that you need there.

Feel free to let me know if you want me to look into it for you.

Think I got something going with it but feel free to push any changes if you want to do it in a different way, I'm not overly confident with go. I also fixed (and added a test case) for functions and null columns, I wasn't happy with removing the codes.InvalidArgument but felt it was okay, shout if you think of a better way to do it (or feel free to push commits)

olavloite · 2021-09-15T15:09:21Z

@psytale I decided to remove the extra error wrapping and checking, and instead add NULL checks to the most used evaluations. That should ensure that generated columns that evaluate to NULL will (in most cases) work as intended. The cases where they don't are not really related to the generated columns implementation, but to the fact that those evaluations would fail anyways before this change. So if you were to for example do a SELECT a*b FROM Foo where some values of a or b are null, the select would fail.

@hengfengli Would you mind taking a quick look at this as well?

hengfengli · 2021-09-17T06:30:01Z

spanner/spannertest/db.go

+		// generated columns with fresh data.
+		pk := r[:t.pkCols]
+		rowNum, found := t.rowForPK(pk)
+		// this should never fail as the row was just inserted


this -> This

Also, the missing period at the end ... inserted.

hengfengli · 2021-09-17T06:31:25Z

spanner/spannertest/db.go

+			row:  row,
+		}
+
+		// TODO: We would need to do a topological sort on dependencies (i.e. what other columns the expression references)


I think we have a convention to limit the line length of comments to 80 characters.

hengfengli · 2021-09-17T06:33:01Z

spanner/spannertest/db.go

@@ -643,6 +677,9 @@ func (t *table) addColumn(cd spansql.ColumnDef, newTable bool) *status.Status {
 		Name:    cd.Name,
 		Type:    cd.Type,
 		NotNull: cd.NotNull,
+		// TODO: We should figure out what columns the Generator expression relies on and check validate it at this time


Please well format the comment.

hengfengli · 2021-09-17T06:43:47Z

spanner/spannertest/integration_test.go

+		t.Errorf("Read rows failed: %v", err)
+	}
+
+	// Great writer has nil because NumSongs is nil


The comment should be Poor writer, right?

hengfengli · 2021-09-17T21:37:11Z

@olavloite I saw some of your comments in the tests are not resolved yet. Do you think they should be fixed before merging?

…o into generated-columns

olavloite · 2021-09-20T07:33:37Z

@olavloite I saw some of your comments in the tests are not resolved yet. Do you think they should be fixed before merging?

I've added the missing error message and missing delete test.

My comment on tests for dropping and altering columns is void, as we have removed the check altogether and added it to the list of limitations in the README.

So as far as I'm concerned, this is now good to be merged once CI is green.

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Sep 10, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 10, 2021

philwitty marked this pull request as ready for review September 10, 2021 13:56

philwitty requested review from hengfengli, skuruppu and a team as code owners September 10, 2021 13:56

hengfengli requested a review from olavloite September 13, 2021 01:08

olavloite requested changes Sep 13, 2021

View reviewed changes

philwitty added 2 commits September 13, 2021 10:06

feat(spanner/spannertest): Support generated columns

d70a2fe

Add support for LOWER function

1469ccd

philwitty force-pushed the generated-columns branch from 777d73d to 1469ccd Compare September 13, 2021 08:06

philwitty added 2 commits September 14, 2021 14:14

Merge remote-tracking branch 'origin/master' into generated-columns

8e160e7

Remove checking of dependent columns and improve testing

d247e11

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2021

olavloite reviewed Sep 14, 2021

View reviewed changes

docs: fix a couple of comments

023d966

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 14, 2021

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 14, 2021

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2021

olavloite approved these changes Sep 14, 2021

View reviewed changes

Make CI happy with casing

3babbb9

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2021

philwitty added 2 commits September 15, 2021 10:21

Add ErrorIs and make functions public to allow spannertest to use

af15328

olavloite added 2 commits September 15, 2021 15:01

fix: add errorIs to spannertest package

f55835d

fix: remove unrelated changes

8f53cad

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 15, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 15, 2021

olavloite added 3 commits September 15, 2021 16:34

fix: remove specific error checking and return null from evaluations

e3c82c2

fix: restore to original errors

2bf349c

Merge branch 'master' into generated-columns

1c9fe2c

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 15, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 15, 2021

hengfengli reviewed Sep 17, 2021

View reviewed changes

Prettify comments

03d80d3

olavloite added 3 commits September 20, 2021 09:31

test: add error msg + delete test

d33754c

Merge branch 'generated-columns' of github.com:psytale/google-cloud-g…

e04ef36

…o into generated-columns

Merge branch 'master' into generated-columns

99a8c91

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 20, 2021

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 20, 2021

hengfengli approved these changes Sep 20, 2021

View reviewed changes

hengfengli merged commit 324d11d into googleapis:master Sep 20, 2021

	// TODO: We would need to do a topoligical sort on dependencies to ensure we can
	// TODO: We would need to do a topological sort on dependencies to ensure we can

feat(spanner/spannertest): Support generated columns #4742

feat(spanner/spannertest): Support generated columns #4742

Conversation

philwitty commented Sep 10, 2021

olavloite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philwitty commented Sep 13, 2021

philwitty commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Sep 14, 2021

olavloite commented Sep 14, 2021

olavloite left a comment

Choose a reason for hiding this comment

olavloite commented Sep 15, 2021

philwitty commented Sep 15, 2021

olavloite commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hengfengli commented Sep 17, 2021

olavloite commented Sep 20, 2021