sql: add typecheck and validate of SET CLUSTER SETTING

pkg/sql/logic_test.go

@@ -46,6 +46,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -1433,6 +1434,10 @@ func (t *logicTest) run() {
 func TestLogic(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
+	// Needed for settings logic tests.
+	_, _, cleanup := settings.TestingAddTestVars()
+	defer cleanup()
+
 	if testutils.Stress() {
 		t.Skip()
 	}

pkg/sql/set.go

@@ -25,6 +25,7 @@ import (
 	"golang.org/x/net/context"
 
 	"github.com/cockroachdb/apd"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -93,6 +94,11 @@ func (p *planner) setClusterSetting(
 	if err := p.RequireSuperUser("SET CLUSTER SETTING"); err != nil {
 		return nil, err
 	}
+	typ, ok := settings.TypeOf(name)
+	if !ok {
+		return nil, errors.Errorf("unknown cluster setting '%s'", name)
+	}
+
 	name = strings.ToLower(name)
 	ie := InternalExecutor{LeaseManager: p.LeaseMgr()}
 
@@ -105,7 +111,7 @@ func (p *planner) setClusterSetting(
 		}
 	case 1:
 		// TODO(dt): validate and properly encode str according to type.
-		encoded, err := p.toSettingString(v[0])
+		encoded, err := p.toSettingString(name, typ, v[0])
 		if err != nil {
 			return nil, err
 		}
@@ -121,17 +127,53 @@ func (p *planner) setClusterSetting(
 	return &emptyNode{}, nil
 }
 
-func (p *planner) toSettingString(raw parser.Expr) (string, error) {
-	// TODO(dt): typecheck and handle according to setting's desired type.
-	typed, err := parser.TypeCheckAndRequire(raw, nil, parser.TypeString, "SET")
-	if err != nil {
-		return "", err
+func (p *planner) toSettingString(
+	name string, typ settings.ValueType, raw parser.Expr,
+) (string, error) {
+	typeCheckAndParse := func(t parser.Type, f func(parser.Datum) (string, error)) (string, error) {
+		typed, err := parser.TypeCheckAndRequire(raw, nil, t, name)
+		if err != nil {
+			return "", err
+		}
+		d, err := typed.Eval(&p.evalCtx)
+		if err != nil {
+			return "", err
+		}
+		return f(d)
 	}
-	d, err := typed.Eval(&p.evalCtx)
-	if err != nil {
-		return "", err
+
+	switch typ {
+	case settings.StringValue:
+		return typeCheckAndParse(parser.TypeString, func(d parser.Datum) (string, error) {
+			if s, ok := d.(*parser.DString); ok {
+				return string(*s), nil
+			}
+			return "", errors.Errorf("cannot use %s %T value for string setting", d.ResolvedType(), d)
+		})
+	case settings.BoolValue:
+		return typeCheckAndParse(parser.TypeBool, func(d parser.Datum) (string, error) {
+			if b, ok := d.(*parser.DBool); ok {
+				return settings.EncodeBool(bool(*b)), nil
+			}
+			return "", errors.Errorf("cannot use %s %T value for bool setting", d.ResolvedType(), d)
+		})
+	case settings.IntValue:
+		return typeCheckAndParse(parser.TypeInt, func(d parser.Datum) (string, error) {
+			if i, ok := d.(*parser.DInt); ok {
+				return settings.EncodeInt(int(*i)), nil
+			}
+			return "", errors.Errorf("cannot use %s %T value for int setting", d.ResolvedType(), d)
+		})
+	case settings.FloatValue:
+		return typeCheckAndParse(parser.TypeFloat, func(d parser.Datum) (string, error) {
+			if f, ok := d.(*parser.DFloat); ok {
+				return settings.EncodeFloat(float64(*f)), nil
+			}
+			return "", errors.Errorf("cannot use %s %T value for float setting", d.ResolvedType(), d)
+		})
+	default:
+		return "", errors.Errorf("unsupported setting type %c", typ)
 	}
-	return string(parser.MustBeDString(d)), nil
 }
 
 func (p *planner) getStringVal(name string, values []parser.TypedExpr) (string, error) {

pkg/sql/testdata/logic_test/set

@@ -198,7 +198,7 @@ statement error variable "max_index_keys" cannot be changed
 SET max_index_keys = 32
 
 query TT
-SELECT name, value FROM system.settings WHERE name = 'foo'
+SELECT name, value FROM system.settings WHERE name = 'testing.str'
 ----
 
 user testuser
@@ -208,39 +208,55 @@ SET CLUSTER SETTING foo = 'bar'
 user root
 
 query TT
-SELECT name, value FROM system.settings WHERE name = 'foo'
+SELECT name, value FROM system.settings WHERE name = 'testing.str'
 ----
 
 statement ok
-SET CLUSTER SETTING foo = 'bar'
+SET CLUSTER SETTING testing.str = 'bar'
 
 query TT
-SELECT name, value FROM system.settings WHERE name = 'foo'
+SELECT name, value FROM system.settings WHERE name = 'testing.str'
 ----
-foo   bar
+testing.str   bar
 
 statement ok
-SET CLUSTER SETTING foo = 'baz'
+SET CLUSTER SETTING testing.str = 'baz'
 
 query TT
-SELECT name, value FROM system.settings WHERE name = 'foo'
+SELECT name, value FROM system.settings WHERE name = 'testing.str'
 ----
-foo   baz
+testing.str   baz
 
 user testuser
 statement error only root is allowed to SET CLUSTER SETTING
-SET CLUSTER SETTING foo = 'bar'
+SET CLUSTER SETTING testing.str = 'bar'
 
 user root
 
 query TT
-SELECT name, value FROM system.settings WHERE name = 'foo'
+SELECT name, value FROM system.settings WHERE name = 'testing.str'
 ----
-foo   baz
+testing.str   baz
 
 statement ok
-SET CLUSTER SETTING foo TO DEFAULT
+SET CLUSTER SETTING testing.str TO DEFAULT
+
+query TT
+SELECT name, value FROM system.settings WHERE name = 'testing.str'
+----
+
+statement ok
+SET CLUSTER SETTING testing.int TO 5
+
+query TT
+SELECT name, value FROM system.settings WHERE name = 'testing.int'
+----
+testing.int 5
+
+statement error argument of testing.int must be type int, not type string
+SET CLUSTER SETTING testing.int TO 'hello'
 
 query TT
-SELECT name, value FROM system.settings WHERE name = 'foo'
+SELECT name, value FROM system.settings WHERE name = 'testing.int'
 ----
+testing.int 5