opt: add testing for operator assumptions

[rytaft]

In operator.go there are a number of functions including ScalarOperatorTransmitsNulls, BoolOperatorRequiresNotNullArgs, AggregateIgnoresNulls, AggregateIsNullOnEmpty, AggregateIsNeverNullOnNonNullInput, AggregateIsNeverNull, AggregatesCanMerge, and AggregateIgnoresDuplicates that serve as facts about different operators. However, for the most part, these facts are not verified anywhere. For at least some of them, we could add some tests similar to those in typing_test.go which test that our assumptions are indeed true.

Aggregate builtins seem to be a popular area for external contributors to work on, so we should make it less easy to make mistakes when updating the functions in operator.go.

Jira issue: CRDB-3624

[idnaninitesh]

Hello @rytaft, I see that most of those functions are being tested in the operator_test.go file, let me know if we need to add something else and I can probably do that.

[rytaft]

Hi @idnaninitesh,

That's great!

You are right that there is one existing test, TestAggregateProperties, which is testing that all aggregate operators are accounted for in each of the functions in operator.go that affect aggregates. However, it doesn't test that the value returned for each of those aggregate operators is correct.

What I am envisioning is that we would add some tests that validate the correctness of the returned value. For example, we could test AggregateIsNullOnEmpty by passing empty input to each of the aggregate operators, and validate that AggregateIsNullOnEmpty returns true if the aggregate returns null, and false if the aggregate does not return null. For AggregateIgnoresNulls, we could pass input with some nulls and the same input with nulls removed to each aggregate operator, and validate that AggregateIgnoresNulls returns true if the two invocations return the same output, and false otherwise.

It may be more difficult to test some of the other functions, but I think we can test a large percentage of them by constructing inputs to the aggregate operators in this way, and testing that the function returns the expected result. We should also be able to test ScalarOperatorTransmitsNulls and BoolOperatorRequiresNotNullArgs using a similar method.

In order to test ScalarOperatorTransmitsNulls and BoolOperatorRequiresNotNullArgs, you should be able to use execbuilder.BuildScalar(), similar to how it's used in eval_test.go. For the aggregates, it will probably be easiest to run a SQL statement with the aggregate function and check the output. See stats.TestDefaultColumns for an example of how to run SQL statements and check the output from a test. My only concern with that approach is that we need to make sure that the aggregate is actually being executed in each case and not optimized away. I don't think that should be a problem, but it's something to keep in mind.

Please let me know if this still seems like something you'd like to take on, and if you have any questions.

Thanks!

[LuiGiovanni]

Hello @rytaft, if no one will. I would love to tackle this.

[rytaft]

Hi @LuiGiovanni, it's all yours! Let me know if you need any additional pointers beyond what I've written above. Thank you!

[LuiGiovanni]

@rytaft thank you! I'll try and give a solution and send you a PR today to receive some feedback on what I'm doing, I'll get on it right away!

[LuiGiovanni]

Hi there @rytaft I must be doing something wrong here, but when I run go test to see the results I'm receiving this output

I've been following a lot of your wiki guides and I tried joining your slack community still haven't been able to, maybe there is something I am missing? I would appreciate your help, thank you!

[rytaft]

Hi @LuiGiovanni,

You should be able to join the community slack here: https://cockroa.ch/slack. Is that link not working for you?

In order to fix the error you've printed above, try using make instead of go test, since that will build all of the necessary dependencies. You could either run make buildshort and then try the command you printed above again, or you can just run the test directly with make test PKG=./pkg/sql/opt TESTS='<test name>'. For example, make test PKG=./pkg/sql/opt TESTS='TestAggregateProperties'.

Does that fix the error for you?

[LuiGiovanni]

@rytaft Hi there!! the link asks me to have a cockroachlabs email, which I don't have.

Yes I did try running make receiving the next output

I was following most of the wiki yesterday.

[rytaft]

Hi @LuiGiovanni,

Sorry about the issue with Slack -- we're working on fixing it, and I'll let you know when you can try again.

In the mean time, based on the error above, it looks like you might need to adjust your directory structure. From the wiki:

Get the CockroachDB code

mkdir -p $(go env GOPATH)/src/github.com/cockroachdb
cd $(go env GOPATH)/src/github.com/cockroachdb
git clone https://github.com/cockroachdb/cockroach
cd cockroach

Note: it is important to ensure the CockroachDB sources are positioned correctly relative to $GOPATH. Otherwise, the builds will fail.

[rytaft]

Hi @LuiGiovanni, This link should work for you to join the Community Slack: https://go.crdb.dev/p/slack Please let me know if you have any issues with this link.

[LuiGiovanni]

@rytaft thank you!!! Worked perfectly

[Abhijeetiyengar]

@rytaft @RaduBerinde . I have created a PR for this https://github.com/cockroachdb/cockroach/pull/64911 . I am new to go and cockroach db too, so happy to take any comments.

[github-actions[bot]]

We have marked this issue as stale because it has been inactive for 18 months. If this issue is still relevant, removing the stale label or adding a comment will keep it active. Otherwise, we'll close it in 10 days to keep the issue queue tidy. Thank you for your contribution to CockroachDB!