Merge #25722

25722: sql: Split SqlLite tests from main TestLogic tests r=andy-kimball a=andy-kimball

During our nightly runs, we pass the -bigtest flag to the main
LogicTest. This causes it to run a completely different set of
tests, pulled from a fork of:

  https://github.com/cockroachdb/sqllogictest

This commit segregates those tests into a new top level go test,
called TestSqlLiteLogic. The -bigtest flag is still required in
order to run the test, since it takes too long to run by default.

This commit prepares the way for separating out other aspects of
the monolithic TestLogic test, such as variations that are intended
to test the 2.0 heuristic planner. Eventually, we will get to a
point where TestLogic only tests the correctness of relational
output, rather than the specifics of how that output was derived.
This will allow the 2.0 heuristic planner and the 2.1 cost-based
optimizer to share these tests.

Other aspects of our testing that cannot be easily shared can go
into other top-level tests.

Release note: None

Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>

build/teamcity-sqllogictest.sh

@@ -3,15 +3,15 @@ set -euxo pipefail
 
 mkdir -p artifacts
 
-# make bigtest needs the sqllogictest repo from the host's GOPATH, so we can't
-# hide it like we do in the other teamcity build scripts.
+# TestSqlLiteLogic needs the sqllogictest repo from the host's GOPATH, so we
+# can't hide it like we do in the other teamcity build scripts.
 # TODO(jordan) improve builder.sh to allow partial GOPATH hiding rather than
 # the all-on/all-off strategy BULIDER_HIDE_GOPATH_SRC gives us.
 export BUILDER_HIDE_GOPATH_SRC=0
 
 for config in default distsql distsql-disk; do
     build/builder.sh env \
-        make test TESTFLAGS="-v -bigtest -config ${config}" TESTTIMEOUT='24h' PKG='./pkg/sql/logictest' TESTS='^TestLogic$$' 2>&1 \
+        make test TESTFLAGS="-v -bigtest -config ${config}" TESTTIMEOUT='24h' PKG='./pkg/sql/logictest' TESTS='^TestSqlLiteLogic$$' 2>&1 \
         | tee "artifacts/${config}.log" \
         | go-test-teamcity
 done

pkg/ccl/logictestccl/logic_test.go

@@ -18,5 +18,5 @@ import (
 
 func TestCCLLogic(t *testing.T) {
 	defer leaktest.AfterTest(t)()
-	logictest.RunLogicTest(t)
+	logictest.RunLogicTest(t, "testdata/logic_test/[^.]*")
 }

pkg/sql/logictest/logic.go

@@ -22,7 +22,6 @@ import (
 	gosql "database/sql"
 	"flag"
 	"fmt"
-	"go/build"
 	"io"
 	"net/url"
 	"os"
@@ -81,16 +80,15 @@ import (
 //
 // Input files for unit testing are stored alongside the source code
 // in the `testdata` subdirectory. The input files for the larger
-// `bigtest` are stored in a separate repository.
+// TestSqlLiteLogic tests are stored in a separate repository.
 //
 // The test input is expressed using a domain-specific language, called
 // Test-Script, defined by SQLite's "Sqllogictest".  The official home
 // of Sqllogictest and Test-Script is
 //      https://www.sqlite.org/sqllogictest/
 //
-// (CockroachDB's `bigtest` is actually a fork of the Sqllogictest
-// test files; its input files are hosted at
-// https://github.com/cockroachdb/sqllogictest )
+// (the TestSqlLiteLogic test uses a fork of the Sqllogictest test files;
+// its input files are hosted at https://github.com/cockroachdb/sqllogictest)
 //
 // Test-Script is line-oriented. It supports both statements which
 // generate no result rows, and queries that produce result rows. The
@@ -118,11 +116,11 @@ import (
 //      CREATE TABLE kv (k INT PRIMARY KEY, v INT)
 //
 //
-// - statement count N
-//   Like "statement ok" but expect a final RowsAffected count of N.
-//   example:
-//     statement count 2
-//     INSERT INTO kv VALUES (1,2), (2,3)
+//  - statement count N
+//    Like "statement ok" but expect a final RowsAffected count of N.
+//    example:
+//      statement count 2
+//      INSERT INTO kv VALUES (1,2), (2,3)
 //
 //  - statement error <regexp>
 //    Runs the statement that follows and expects an
@@ -247,8 +245,8 @@ import (
 // -d <glob>  selects all files matching <glob>. This can mix and
 //            match wildcards (*/?) or groups like {a,b,c}.
 //
-// -bigtest   cancels any -d setting and selects all relevant input
-//            files from CockroachDB's fork of Sqllogictest.
+// -bigtest   enable the long-running SqlLiteLogic test, which uses files from
+//            CockroachDB's fork of Sqllogictest.
 //
 // Configuration:
 //
@@ -315,10 +313,8 @@ var (
 	varRE     = regexp.MustCompile(`\$[a-zA-Z][a-zA-Z_0-9]*`)
 
 	// Input selection
-	logictestdata = flag.String("d", "testdata/logic_test/[^.]*", "test data glob")
-	bigtest       = flag.Bool(
-		"bigtest", false, "use the big set of logic test files (overrides testdata)",
-	)
+	logictestdata = flag.String("d", "", "glob that selects subset of files to run")
+	bigtest       = flag.Bool("bigtest", false, "enable the long-running SqlLiteLogic test")
 	defaultConfig = flag.String(
 		"config", "default",
 		"customizes the default test cluster configuration for files that lack LogicTest directives",
@@ -1858,8 +1854,9 @@ var skipLogicTests = envutil.EnvOrDefaultBool("COCKROACH_LOGIC_TESTS_SKIP", fals
 var logicTestsConfigExclude = envutil.EnvOrDefaultString("COCKROACH_LOGIC_TESTS_SKIP_CONFIG", "")
 var logicTestsConfigFilter = envutil.EnvOrDefaultString("COCKROACH_LOGIC_TESTS_CONFIG", "")
 
-// RunLogicTest is the main entry point for the logic test.
-func RunLogicTest(t *testing.T) {
+// RunLogicTest is the main entry point for the logic test. The globs parameter
+// specifies the default sets of files to run.
+func RunLogicTest(t *testing.T, globs ...string) {
 	if testutils.NightlyStress() {
 		// See https://github.com/cockroachdb/cockroach/pull/10966.
 		t.Skip()
@@ -1869,51 +1866,15 @@ func RunLogicTest(t *testing.T) {
 		t.Skip("COCKROACH_LOGIC_TESTS_SKIP")
 	}
 
+	// Override default glob sets if -d flag was specified.
+	if *logictestdata != "" {
+		globs = []string{*logictestdata}
+	}
+
 	// Set time.Local to time.UTC to circumvent pq's timetz parsing flaw.
 	time.Local = time.UTC
 
-	// run the logic tests indicated by the bigtest and logictestdata flags.
 	// A new cluster is set up for each separate file in the test.
-	var globs []string
-	if *bigtest {
-		logicTestPath := build.Default.GOPATH + "/src/github.com/cockroachdb/sqllogictest"
-		if _, err := os.Stat(logicTestPath); os.IsNotExist(err) {
-			fullPath, err := filepath.Abs(logicTestPath)
-			if err != nil {
-				t.Fatal(err)
-			}
-			t.Fatalf("unable to find sqllogictest repo: %s\n"+
-				"git clone https://github.com/cockroachdb/sqllogictest %s",
-				logicTestPath, fullPath)
-			return
-		}
-		globs = []string{
-			logicTestPath + "/test/index/between/*/*.test",
-			logicTestPath + "/test/index/commute/*/*.test",
-			logicTestPath + "/test/index/delete/*/*.test",
-			logicTestPath + "/test/index/in/*/*.test",
-			logicTestPath + "/test/index/orderby/*/*.test",
-			logicTestPath + "/test/index/orderby_nosort/*/*.test",
-			logicTestPath + "/test/index/view/*/*.test",
-
-			// TODO(pmattis): Incompatibilities in numeric types.
-			// For instance, we type SUM(int) as a decimal since all of our ints are
-			// int64.
-			// logicTestPath + "/test/random/expr/*.test",
-
-			// TODO(pmattis): We don't support correlated subqueries.
-			// logicTestPath + "/test/select*.test",
-
-			// TODO(pmattis): We don't support unary + on strings.
-			// logicTestPath + "/test/index/random/*/*.test",
-			// [uses joins] logicTestPath + "/test/random/aggregates/*.test",
-			// [uses joins] logicTestPath + "/test/random/groupby/*.test",
-			// [uses joins] logicTestPath + "/test/random/select/*.test",
-		}
-	} else {
-		globs = []string{*logictestdata}
-	}
-
 	var paths []string
 	for _, g := range globs {
 		match, err := filepath.Glob(g)

pkg/sql/logictest/logic_test.go

@@ -15,12 +15,82 @@
 package logictest
 
 import (
+	"go/build"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 )
 
+// TestLogic runs logic tests that were written by hand to test various
+// CockroachDB features. The tests use a similar methodology to the SQLLite
+// Sqllogictests.
+//
+// See the comments in logic.go for more details.
 func TestLogic(t *testing.T) {
 	defer leaktest.AfterTest(t)()
-	RunLogicTest(t)
+	RunLogicTest(t, "testdata/logic_test/[^.]*")
+}
+
+// TestSqlLiteLogic runs all logic tests from CockroachDB's fork of
+// Sqllogictest:
+//
+//   https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki
+//
+// This fork contains many generated tests created by the SqlLite project that
+// ensure the tested SQL database returns correct statement and query output.
+// The logic tests are reasonably independent of the specific dialect of each
+// database so that they can be easily retargeted. In fact, the expected output
+// for each test can be generated by one database and then used to verify the
+// output of another database.
+//
+// By default, this test is skipped, unless the `bigtest` flag is specified.
+// The reason for this is that these tests are contained in another repo that
+// must be present on the machine, and because they take a long time to run.
+//
+// See the comments in logic.go for more details.
+func TestSqlLiteLogic(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	if !*bigtest {
+		t.Skip("-bigtest flag must be specified to run this test")
+	}
+
+	logicTestPath := build.Default.GOPATH + "/src/github.com/cockroachdb/sqllogictest"
+	if _, err := os.Stat(logicTestPath); os.IsNotExist(err) {
+		fullPath, err := filepath.Abs(logicTestPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Fatalf("unable to find sqllogictest repo: %s\n"+
+			"git clone https://github.com/cockroachdb/sqllogictest %s",
+			logicTestPath, fullPath)
+		return
+	}
+	globs := []string{
+		logicTestPath + "/test/index/between/*/*.test",
+		logicTestPath + "/test/index/commute/*/*.test",
+		logicTestPath + "/test/index/delete/*/*.test",
+		logicTestPath + "/test/index/in/*/*.test",
+		logicTestPath + "/test/index/orderby/*/*.test",
+		logicTestPath + "/test/index/orderby_nosort/*/*.test",
+		logicTestPath + "/test/index/view/*/*.test",
+
+		// TODO(pmattis): Incompatibilities in numeric types.
+		// For instance, we type SUM(int) as a decimal since all of our ints are
+		// int64.
+		// logicTestPath + "/test/random/expr/*.test",
+
+		// TODO(pmattis): We don't support correlated subqueries.
+		// logicTestPath + "/test/select*.test",
+
+		// TODO(pmattis): We don't support unary + on strings.
+		// logicTestPath + "/test/index/random/*/*.test",
+		// [uses joins] logicTestPath + "/test/random/aggregates/*.test",
+		// [uses joins] logicTestPath + "/test/random/groupby/*.test",
+		// [uses joins] logicTestPath + "/test/random/select/*.test",
+	}
+
+	RunLogicTest(t, globs...)
 }