sqlsmith: Add aggregation TLP testing

[nehageorge]

Previously, TLP testing included JOINS and WHERE clauses. In
this PR, this is expanded to include MAX and MIN based testing.

Release note: None

[cockroach-teamcity]

This change is 

[rytaft]

Nice!

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nehageorge)

---

pkg/internal/sqlsmith/tlp.go, line 278 at r1 (raw file):

// that are supported are MAX() and MIN(), although SUM(), AVG(), and COUNT()
// are also valid TLP aggregations.

nit: add // on this line so it's obvious the comment is continuous

---

pkg/internal/sqlsmith/tlp.go, line 303 at r1 (raw file):

//
// If the resulting counts of the two queries are not equal, there is a logical
// bug.

Are we interested in counts in this case? Or rather the value of min/max?

If the value of min/max, what happens if first is not an integer column? Won't runTLPQuery fail since it expects an int?

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nehageorge and @rytaft)

---

pkg/internal/sqlsmith/tlp.go, line 303 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Are we interested in counts in this case? Or rather the value of min/max?

If the value of min/max, what happens if first is not an integer column? Won't runTLPQuery fail since it expects an int?

Good catch on the comment, thanks! Yep, you're right about the second part. I thought of changing this but I decided that it's not worth it since in the PR I'm working on effectively reverses this change. In that PR, the counts are no longer compared and instead the results are compared directly with everything cast to a string. So it will be cast to a string in the future. For now, the scan will fail for non-numerical results. But in that PR, the scan will always succeed since it's a string being scanned into a string type variable. Let me know what you think of this.

[rytaft]

---

pkg/internal/sqlsmith/tlp.go, line 303 at r1 (raw file):

Previously, nehageorge wrote…

Good catch on the comment, thanks! Yep, you're right about the second part. I thought of changing this but I decided that it's not worth it since in the PR I'm working on effectively reverses this change. In that PR, the counts are no longer compared and instead the results are compared directly with everything cast to a string. So it will be cast to a string in the future. For now, the scan will fail for non-numerical results. But in that PR, the scan will always succeed since it's a string being scanned into a string type variable. Let me know what you think of this.

Ok, that sounds fine, since it looks like runTLPQuery would just ignore the error. I just wouldn't want you to merge this and have it cause a bunch of spurious test failures, but it looks like that won't be a problem.

[nehageorge]

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

---

pkg/internal/sqlsmith/tlp.go, line 278 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: add // on this line so it's obvious the comment is continuous

Done

---

pkg/internal/sqlsmith/tlp.go, line 303 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Ok, that sounds fine, since it looks like runTLPQuery would just ignore the error. I just wouldn't want you to merge this and have it cause a bunch of spurious test failures, but it looks like that won't be a problem.

Makes sense

[rytaft]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nehageorge)

[nehageorge]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)