From 42dbf0e48559f7002d947e9114557d4b0d6da559 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Wed, 8 Sep 2021 17:20:11 -0700 Subject: [PATCH 01/12] Give a new context to every query in the SQL shell. Unfortunately, SIGTERM still cancels the global parent context, so we need something better at the top level. --- go/cmd/dolt/commands/sql.go | 164 +++++++++++++++++++++++++----------- 1 file changed, 115 insertions(+), 49 deletions(-) diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go index afb3c01001..45d9a445d0 100644 --- a/go/cmd/dolt/commands/sql.go +++ b/go/cmd/dolt/commands/sql.go @@ -19,10 +19,12 @@ import ( "fmt" "io" "os" + "os/signal" "path/filepath" "regexp" "runtime" "strings" + "syscall" "github.com/abiosoft/readline" sqle "github.com/dolthub/go-mysql-server" @@ -367,12 +369,12 @@ func execShell( format resultFormat, ) errhand.VerboseError { dbs := CollectDBs(mrEnv) - se, sqlCtx, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + se, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) if err != nil { return errhand.VerboseErrorFromError(err) } - err = runShell(sqlCtx, se, mrEnv, roots) + err = runShell(ctx, se, mrEnv, roots) if err != nil { return errhand.BuildDError(err.Error()).Build() } @@ -390,7 +392,12 @@ func execBatch( format resultFormat, ) errhand.VerboseError { dbs := CollectDBs(mrEnv) - se, sqlCtx, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + se, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + if err != nil { + return errhand.VerboseErrorFromError(err) + } + + sqlCtx, err := se.newContext(ctx) if err != nil { return errhand.VerboseErrorFromError(err) } @@ -427,7 +434,17 @@ func execMultiStatements( format resultFormat, ) errhand.VerboseError { dbs := CollectDBs(mrEnv) - se, sqlCtx, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + se, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + if err != nil { + return errhand.VerboseErrorFromError(err) + } + + sqlCtx, err := se.newContext(ctx) + if err != nil { + return errhand.VerboseErrorFromError(err) + } + + err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) if err != nil { return errhand.VerboseErrorFromError(err) } @@ -455,7 +472,17 @@ func execQuery( format resultFormat, ) errhand.VerboseError { dbs := CollectDBs(mrEnv) - se, sqlCtx, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + se, err := newSqlEngine(ctx, dEnv, mrEnv, roots, readOnly, format, dbs...) + if err != nil { + return errhand.VerboseErrorFromError(err) + } + + sqlCtx, err := se.newContext(ctx) + if err != nil { + return errhand.VerboseErrorFromError(err) + } + + err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) if err != nil { return errhand.VerboseErrorFromError(err) } @@ -738,14 +765,20 @@ func runBatchMode(ctx *sql.Context, se *sqlEngine, input io.Reader, continueOnEr // runShell starts a SQL shell. Returns when the user exits the shell. The Root of the sqlEngine may // be updated by any queries which were processed. -func runShell(ctx *sql.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initialRoots map[string]*doltdb.RootValue) error { +func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initialRoots map[string]*doltdb.RootValue) error { _ = iohelp.WriteLine(cli.CliOut, welcomeMsg) - currentDB := ctx.Session.GetCurrentDatabase() + + sqlCtx, err := se.newContext(ctx) + if err != nil { + return err + } + + currentDB := sqlCtx.Session.GetCurrentDatabase() currEnv := mrEnv[currentDB] // start the doltsql shell historyFile := filepath.Join(".sqlhistory") // history file written to working dir - initialPrompt := fmt.Sprintf("%s> ", ctx.GetCurrentDatabase()) + initialPrompt := fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) initialMultilinePrompt := fmt.Sprintf(fmt.Sprintf("%%%ds", len(initialPrompt)), "-> ") rlConf := readline.Config{ @@ -823,18 +856,32 @@ func runShell(ctx *sql.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initialRo query = query[:len(query)-len(shell.LineTerminator())] } - if sqlSch, rowIter, err = processQuery(ctx, query, se); err != nil { + subCtx, cancelF := context.WithCancel(ctx) + sqlCtx, err = se.newContext(subCtx) + if err != nil { + shell.Println(color.RedString(err.Error())) + return + } + + c := make(chan os.Signal) + signal.Notify(c, os.Interrupt, syscall.SIGTERM) + go func() { + <-c + cancelF() + }() + + if sqlSch, rowIter, err = processQuery(sqlCtx, query, se); err != nil { verr := formatQueryError("", err) shell.Println(verr.Verbose()) } else if rowIter != nil { - err = PrettyPrintResults(ctx, se.resultFormat, sqlSch, rowIter, HasTopLevelOrderByClause(query)) + err = PrettyPrintResults(sqlCtx, se.resultFormat, sqlSch, rowIter, HasTopLevelOrderByClause(query)) if err != nil { shell.Println(color.RedString(err.Error())) } } } - currPrompt := fmt.Sprintf("%s> ", ctx.GetCurrentDatabase()) + currPrompt := fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) shell.SetPrompt(currPrompt) shell.SetMultiPrompt(fmt.Sprintf(fmt.Sprintf("%%%ds", len(currPrompt)), "-> ")) }) @@ -1345,10 +1392,12 @@ func mergeResultIntoStats(statement sqlparser.Statement, rowIter sql.RowIter, s } type sqlEngine struct { - dbs map[string]dsqle.Database - mrEnv env.MultiRepoEnv - engine *sqle.Engine - resultFormat resultFormat + dbs map[string]dsqle.Database + mrEnv env.MultiRepoEnv + sess *dsess.Session + contextFactory func(ctx context.Context) (*sql.Context, error) + engine *sqle.Engine + resultFormat resultFormat } var ErrDBNotFoundKind = errors.NewKind("database '%s' not found") @@ -1362,7 +1411,7 @@ func newSqlEngine( readOnly bool, format resultFormat, dbs ...dsqle.Database, -) (*sqlEngine, *sql.Context, error) { +) (*sqlEngine, error) { var au auth.Auth if readOnly { @@ -1377,7 +1426,7 @@ func newSqlEngine( err := cat.Register(dfunctions.DoltFunctions...) if err != nil { - return nil, nil, err + return nil, err } engine := sqle.New(cat, analyzer.NewBuilder(cat).WithParallelism(parallelism).Build(), &sqle.Config{Auth: au}) @@ -1401,7 +1450,7 @@ func newSqlEngine( // since it isn't a current HEAD. dbState, err := getDbState(ctx, db, mrEnv) if err != nil { - return nil, nil, err + return nil, err } dbStates = append(dbStates, dbState) @@ -1412,41 +1461,44 @@ func newSqlEngine( email := *dEnv.Config.GetStringOrDefault(env.UserEmailKey, "") sess, err := dsess.NewSession(sql.NewEmptyContext(), sql.NewBaseSession(), pro, username, email, dbStates...) - sqlCtx := sql.NewContext(ctx, - sql.WithSession(sess), - sql.WithIndexRegistry(sql.NewIndexRegistry()), - sql.WithViewRegistry(sql.NewViewRegistry()), - sql.WithTracer(tracing.Tracer(ctx))) + return &sqlEngine{ + dbs: nameToDB, + mrEnv: mrEnv, + sess: sess, + contextFactory: newSqlContext(sess, cat), + engine: engine, + resultFormat: format, + }, nil +} - for _, db := range dbsAsDSQLDBs(cat.AllDatabases()) { - root, err := db.GetRoot(sqlCtx) - if err != nil { - return nil, nil, err +func newSqlContext(sess *dsess.Session, cat *sql.Catalog) func(ctx context.Context) (*sql.Context, error) { + return func(ctx context.Context) (*sql.Context, error) { + sqlCtx := sql.NewContext(ctx, + sql.WithSession(sess), + sql.WithIndexRegistry(sql.NewIndexRegistry()), + sql.WithViewRegistry(sql.NewViewRegistry()), + sql.WithTracer(tracing.Tracer(ctx))) + + seenOne := false + for _, db := range dbsAsDSQLDBs(cat.AllDatabases()) { + root, err := db.GetRoot(sqlCtx) + if err != nil { + return nil, err + } + + err = dsqle.RegisterSchemaFragments(sqlCtx, db, root) + if err != nil { + return nil, err + } + + if !seenOne { + sqlCtx.SetCurrentDatabase(db.Name()) + seenOne = true + } } - err = dsqle.RegisterSchemaFragments(sqlCtx, db, root) - if err != nil { - return nil, nil, err - } + return sqlCtx, nil } - - err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) - if err != nil { - return nil, nil, err - } - - initialRoots, err := mrEnv.GetWorkingRoots(ctx) - if err != nil { - return nil, nil, err - } - - if len(initialRoots) == 1 { - for name := range initialRoots { - sqlCtx.SetCurrentDatabase(name) - } - } - - return &sqlEngine{nameToDB, mrEnv, engine, format}, sqlCtx, nil } func dbsAsDSQLDBs(dbs []sql.Database) []dsqle.Database { @@ -1542,6 +1594,20 @@ func (se *sqlEngine) getRoots(sqlCtx *sql.Context) (map[string]*doltdb.RootValue return newRoots, nil } +func (se *sqlEngine) newContext(ctx context.Context) (*sql.Context, error) { + sqlCtx, err := se.contextFactory(ctx) + if err != nil { + return nil, err + } + + err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) + if err != nil { + return nil, err + } + + return sqlCtx, nil +} + // Execute a SQL statement and return values for printing. func (se *sqlEngine) query(ctx *sql.Context, query string) (sql.Schema, sql.RowIter, error) { return se.engine.Query(ctx, query) From ca1470283959086389b9ecbe57a82ac883f99394 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Wed, 8 Sep 2021 17:40:15 -0700 Subject: [PATCH 02/12] Added an extension to Command to deal wtih signal handlers --- go/cmd/dolt/cli/command.go | 36 ++++++++++++++++++++++++++++++++---- go/cmd/dolt/commands/sql.go | 7 +++++++ go/cmd/dolt/dolt.go | 11 +---------- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/go/cmd/dolt/cli/command.go b/go/cmd/dolt/cli/command.go index 61594c8d90..787c589670 100644 --- a/go/cmd/dolt/cli/command.go +++ b/go/cmd/dolt/cli/command.go @@ -16,7 +16,10 @@ package cli import ( "context" + "os" + "os/signal" "strings" + "syscall" "github.com/fatih/color" @@ -54,7 +57,7 @@ func hasHelpFlag(args []string) bool { // Command is the interface which defines a Dolt cli command type Command interface { - // Name is returns the name of the Dolt cli command. This is what is used on the command line to invoke the command + // Name returns the name of the Dolt cli command. This is what is used on the command line to invoke the command Name() string // Description returns a description of the command Description() string @@ -64,6 +67,15 @@ type Command interface { CreateMarkdown(fs filesys.Filesys, path, commandStr string) error } +// SignalCommand is an extension of Command that allows commands to install their own signal handlers, rather than use +// the global one (which cancels the global context). +type SignalCommand interface { + Command + + // InstallsSignalHandlers returns whether this command manages its own signal handlers for interruption / termination. + InstallsSignalHandlers() bool +} + // This type is to store the content of a documented command, elsewhere we can transform this struct into // other structs that are used to generate documentation at the command line and in markdown files. type CommandDocumentationContent struct { @@ -153,14 +165,17 @@ func (hc SubCommandHandler) Exec(ctx context.Context, commandStr string, args [] if len(args) > 0 { subCommandStr = strings.ToLower(strings.TrimSpace(args[0])) } + + ctx, cancelF := context.WithCancel(ctx) + for _, cmd := range hc.Subcommands { lwrName := strings.ToLower(cmd.Name()) if lwrName == subCommandStr { - return hc.handleCommand(ctx, commandStr+" "+subCommandStr, cmd, args[1:], dEnv) + return hc.handleCommand(ctx, cancelF, commandStr+" "+subCommandStr, cmd, args[1:], dEnv) } } if hc.Unspecified != nil { - return hc.handleCommand(ctx, commandStr, hc.Unspecified, args, dEnv) + return hc.handleCommand(ctx, cancelF, commandStr, hc.Unspecified, args, dEnv) } if !isHelp(subCommandStr) { @@ -171,7 +186,7 @@ func (hc SubCommandHandler) Exec(ctx context.Context, commandStr string, args [] return 1 } -func (hc SubCommandHandler) handleCommand(ctx context.Context, commandStr string, cmd Command, args []string, dEnv *env.DoltEnv) int { +func (hc SubCommandHandler) handleCommand(ctx context.Context, cancelF context.CancelFunc, commandStr string, cmd Command, args []string, dEnv *env.DoltEnv) int { cmdRequiresRepo := true if rnrCmd, ok := cmd.(RepoNotRequiredCommand); ok { cmdRequiresRepo = rnrCmd.RequiresRepo() @@ -190,6 +205,10 @@ func (hc SubCommandHandler) handleCommand(ctx context.Context, commandStr string ctx = events.NewContextForEvent(ctx, evt) } + if signalCmd, ok := cmd.(SignalCommand); !ok || !signalCmd.InstallsSignalHandlers() { + installSignalHandler(cancelF) + } + ret := cmd.Exec(ctx, commandStr, args, dEnv) if evt != nil { @@ -199,6 +218,15 @@ func (hc SubCommandHandler) handleCommand(ctx context.Context, commandStr string return ret } +func installSignalHandler(cancelF context.CancelFunc) { + c := make(chan os.Signal) + signal.Notify(c, os.Interrupt, syscall.SIGTERM) + go func() { + <-c + cancelF() + }() +} + // CheckEnvIsValid validates that a DoltEnv has been initialized properly and no errors occur during load, and prints // error messages to the user if there are issues with the environment or if errors were encountered while loading it. func CheckEnvIsValid(dEnv *env.DoltEnv) bool { diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go index 45d9a445d0..7e075d84b1 100644 --- a/go/cmd/dolt/commands/sql.go +++ b/go/cmd/dolt/commands/sql.go @@ -122,6 +122,13 @@ type SqlCmd struct { VersionStr string } +// The SQL shell installs its own signal handlers so that you can cancel a running query without and still run a new one. +func (cmd SqlCmd) InstallsSignalHandlers() bool { + return true +} + +var _ cli.SignalCommand = SqlCmd{} + // Name returns the name of the Dolt cli command. This is what is used on the command line to invoke the command func (cmd SqlCmd) Name() string { return "sql" diff --git a/go/cmd/dolt/dolt.go b/go/cmd/dolt/dolt.go index 64a9fef836..bdf07f5052 100644 --- a/go/cmd/dolt/dolt.go +++ b/go/cmd/dolt/dolt.go @@ -21,10 +21,8 @@ import ( _ "net/http/pprof" "os" "os/exec" - "os/signal" "strconv" "strings" - "syscall" "github.com/fatih/color" "github.com/opentracing/opentracing-go" @@ -225,14 +223,7 @@ func runMain() int { warnIfMaxFilesTooLow() - ctx, cancelF := context.WithCancel(context.Background()) - c := make(chan os.Signal) - signal.Notify(c, os.Interrupt, syscall.SIGTERM) - go func() { - <-c - cancelF() - }() - + ctx := context.Background() dEnv := env.Load(ctx, env.GetCurrentUserHomeDir, filesys.LocalFS, doltdb.LocalDirDoltDB, Version) if dEnv.DBLoadError == nil && commandNeedsMigrationCheck(args) { From 074ba091f389110c7a0978fe77506a763be8731f Mon Sep 17 00:00:00 2001 From: zachmu Date: Thu, 9 Sep 2021 16:16:22 +0000 Subject: [PATCH 03/12] [ga-format-pr] Run go/utils/repofmt/format_repo.sh and go/Godeps/update.sh --- go/cmd/dolt/commands/sql.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go index 5c284887fe..86cba487d0 100644 --- a/go/cmd/dolt/commands/sql.go +++ b/go/cmd/dolt/commands/sql.go @@ -1473,12 +1473,12 @@ func newSqlEngine( sess, err := dsess.NewSession(sql.NewEmptyContext(), sql.NewBaseSession(), pro, username, email, dbStates...) return &sqlEngine{ - dbs: nameToDB, - mrEnv: mrEnv, - sess: sess, + dbs: nameToDB, + mrEnv: mrEnv, + sess: sess, contextFactory: newSqlContext(sess, cat), - engine: engine, - resultFormat: format, + engine: engine, + resultFormat: format, }, nil } From e62c6b6e0583d7673ea7b31989afc2912c2b6491 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 9 Sep 2021 10:01:19 -0700 Subject: [PATCH 04/12] Use signal.NotifyContext instead of installing signal handlers manually --- go/cmd/dolt/cli/command.go | 16 +++++----------- go/cmd/dolt/commands/sql.go | 36 ++++++++++++++++-------------------- go/go.mod | 2 +- 3 files changed, 22 insertions(+), 32 deletions(-) diff --git a/go/cmd/dolt/cli/command.go b/go/cmd/dolt/cli/command.go index 787c589670..1ccd2324dc 100644 --- a/go/cmd/dolt/cli/command.go +++ b/go/cmd/dolt/cli/command.go @@ -19,7 +19,6 @@ import ( "os" "os/signal" "strings" - "syscall" "github.com/fatih/color" @@ -205,8 +204,12 @@ func (hc SubCommandHandler) handleCommand(ctx context.Context, cancelF context.C ctx = events.NewContextForEvent(ctx, evt) } + // Certain commands cannot tolerate a top-level signal handler (which cancels the root context) but need to manage + // their own interrupt semantics. if signalCmd, ok := cmd.(SignalCommand); !ok || !signalCmd.InstallsSignalHandlers() { - installSignalHandler(cancelF) + var stop context.CancelFunc + ctx, stop = signal.NotifyContext(ctx, os.Interrupt) + defer stop() } ret := cmd.Exec(ctx, commandStr, args, dEnv) @@ -218,15 +221,6 @@ func (hc SubCommandHandler) handleCommand(ctx context.Context, cancelF context.C return ret } -func installSignalHandler(cancelF context.CancelFunc) { - c := make(chan os.Signal) - signal.Notify(c, os.Interrupt, syscall.SIGTERM) - go func() { - <-c - cancelF() - }() -} - // CheckEnvIsValid validates that a DoltEnv has been initialized properly and no errors occur during load, and prints // error messages to the user if there are issues with the environment or if errors were encountered while loading it. func CheckEnvIsValid(dEnv *env.DoltEnv) bool { diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go index 5c284887fe..028e2dcae4 100644 --- a/go/cmd/dolt/commands/sql.go +++ b/go/cmd/dolt/commands/sql.go @@ -24,7 +24,6 @@ import ( "regexp" "runtime" "strings" - "syscall" "github.com/abiosoft/readline" sqle "github.com/dolthub/go-mysql-server" @@ -867,29 +866,26 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia query = query[:len(query)-len(shell.LineTerminator())] } - subCtx, cancelF := context.WithCancel(ctx) - sqlCtx, err = se.newContext(subCtx) - if err != nil { - shell.Println(color.RedString(err.Error())) - return - } + func() { + subCtx, stop := signal.NotifyContext(ctx, os.Interrupt) + defer stop() - c := make(chan os.Signal) - signal.Notify(c, os.Interrupt, syscall.SIGTERM) - go func() { - <-c - cancelF() - }() - - if sqlSch, rowIter, err = processQuery(sqlCtx, query, se); err != nil { - verr := formatQueryError("", err) - shell.Println(verr.Verbose()) - } else if rowIter != nil { - err = PrettyPrintResults(sqlCtx, se.resultFormat, sqlSch, rowIter, HasTopLevelOrderByClause(query)) + sqlCtx, err = se.newContext(subCtx) if err != nil { shell.Println(color.RedString(err.Error())) + return } - } + + if sqlSch, rowIter, err = processQuery(sqlCtx, query, se); err != nil { + verr := formatQueryError("", err) + shell.Println(verr.Verbose()) + } else if rowIter != nil { + err = PrettyPrintResults(sqlCtx, se.resultFormat, sqlSch, rowIter, HasTopLevelOrderByClause(query)) + if err != nil { + shell.Println(color.RedString(err.Error())) + } + } + }() } currPrompt := fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) diff --git a/go/go.mod b/go/go.mod index df263338dc..9397608419 100644 --- a/go/go.mod +++ b/go/go.mod @@ -83,4 +83,4 @@ require ( replace github.com/dolthub/dolt/go/gen/proto/dolt/services/eventsapi => ./gen/proto/dolt/services/eventsapi -go 1.15 +go 1.16 From 8d2a91571d67ac955fe426676bbc86ec1efebc90 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 9 Sep 2021 10:46:48 -0700 Subject: [PATCH 05/12] Fixed signal lint errors --- go/cmd/dolt/cli/command.go | 8 +++----- go/utils/remotesrv/main.go | 6 ++---- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/go/cmd/dolt/cli/command.go b/go/cmd/dolt/cli/command.go index 1ccd2324dc..f9808171b2 100644 --- a/go/cmd/dolt/cli/command.go +++ b/go/cmd/dolt/cli/command.go @@ -165,16 +165,14 @@ func (hc SubCommandHandler) Exec(ctx context.Context, commandStr string, args [] subCommandStr = strings.ToLower(strings.TrimSpace(args[0])) } - ctx, cancelF := context.WithCancel(ctx) - for _, cmd := range hc.Subcommands { lwrName := strings.ToLower(cmd.Name()) if lwrName == subCommandStr { - return hc.handleCommand(ctx, cancelF, commandStr+" "+subCommandStr, cmd, args[1:], dEnv) + return hc.handleCommand(ctx, commandStr+" "+subCommandStr, cmd, args[1:], dEnv) } } if hc.Unspecified != nil { - return hc.handleCommand(ctx, cancelF, commandStr, hc.Unspecified, args, dEnv) + return hc.handleCommand(ctx, commandStr, hc.Unspecified, args, dEnv) } if !isHelp(subCommandStr) { @@ -185,7 +183,7 @@ func (hc SubCommandHandler) Exec(ctx context.Context, commandStr string, args [] return 1 } -func (hc SubCommandHandler) handleCommand(ctx context.Context, cancelF context.CancelFunc, commandStr string, cmd Command, args []string, dEnv *env.DoltEnv) int { +func (hc SubCommandHandler) handleCommand(ctx context.Context, commandStr string, cmd Command, args []string, dEnv *env.DoltEnv) int { cmdRequiresRepo := true if rnrCmd, ok := cmd.(RepoNotRequiredCommand); ok { cmdRequiresRepo = rnrCmd.RequiresRepo() diff --git a/go/utils/remotesrv/main.go b/go/utils/remotesrv/main.go index 8d426810de..7c5fe86bdc 100644 --- a/go/utils/remotesrv/main.go +++ b/go/utils/remotesrv/main.go @@ -73,10 +73,8 @@ func main() { } func waitForSignal() { - c := make(chan os.Signal) - signal.Notify(c, os.Interrupt) - signal.Notify(c, os.Kill) - + c := make(chan os.Signal, 128) + signal.Notify(c, os.Interrupt, os.Kill) <-c } From d72465e5cb921b619b97889ba9661a8a051a5ad2 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 9 Sep 2021 11:11:28 -0700 Subject: [PATCH 06/12] PR feedback, and fixed bug in autocommit handling --- go/cmd/dolt/cli/command.go | 3 ++- go/cmd/dolt/commands/sql.go | 53 +++++++++++++++++-------------------- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/go/cmd/dolt/cli/command.go b/go/cmd/dolt/cli/command.go index f9808171b2..14c98157fa 100644 --- a/go/cmd/dolt/cli/command.go +++ b/go/cmd/dolt/cli/command.go @@ -19,6 +19,7 @@ import ( "os" "os/signal" "strings" + "syscall" "github.com/fatih/color" @@ -206,7 +207,7 @@ func (hc SubCommandHandler) handleCommand(ctx context.Context, commandStr string // their own interrupt semantics. if signalCmd, ok := cmd.(SignalCommand); !ok || !signalCmd.InstallsSignalHandlers() { var stop context.CancelFunc - ctx, stop = signal.NotifyContext(ctx, os.Interrupt) + ctx, stop = signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) defer stop() } diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go index c428cac5f6..20dc794d29 100644 --- a/go/cmd/dolt/commands/sql.go +++ b/go/cmd/dolt/commands/sql.go @@ -24,6 +24,7 @@ import ( "regexp" "runtime" "strings" + "syscall" "github.com/abiosoft/readline" sqle "github.com/dolthub/go-mysql-server" @@ -451,11 +452,6 @@ func execMultiStatements( return errhand.VerboseErrorFromError(err) } - err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) - if err != nil { - return errhand.VerboseErrorFromError(err) - } - err = runMultiStatementMode(sqlCtx, se, batchInput, continueOnErr) if err != nil { // If we encounter an error, attempt to flush what we have so far to disk before exiting @@ -492,11 +488,6 @@ func execQuery( return errhand.VerboseErrorFromError(err) } - err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) - if err != nil { - return errhand.VerboseErrorFromError(err) - } - sqlSch, rowIter, err := processQuery(sqlCtx, query, se) if err != nil { return formatQueryError("", err) @@ -822,6 +813,8 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia c.Stop() }) + // The shell's interrupt handler handles an interrupt that occurs when it's accepting input. We also install our own + // that handles interrupts during query execution or result printing, see below. shell.Interrupt(func(c *ishell.Context, count int, input string) { if count > 1 { c.Stop() @@ -830,7 +823,6 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia } }) - var returnedVerr errhand.VerboseError = nil // Verr that cannot be just printed but needs to be returned. shell.Uninterpreted(func(c *ishell.Context) { query := c.Args[0] if len(strings.TrimSpace(query)) == 0 { @@ -856,6 +848,7 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia shouldProcessQuery = false } + var nextPrompt string if shouldProcessQuery { var sqlSch sql.Schema var rowIter sql.RowIter @@ -866,14 +859,14 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia query = query[:len(query)-len(shell.LineTerminator())] } - func() { - subCtx, stop := signal.NotifyContext(ctx, os.Interrupt) + cont := func() bool { + subCtx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) defer stop() sqlCtx, err = se.newContext(subCtx) if err != nil { shell.Println(color.RedString(err.Error())) - return + return false } if sqlSch, rowIter, err = processQuery(sqlCtx, query, se); err != nil { @@ -885,18 +878,24 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia shell.Println(color.RedString(err.Error())) } } + + nextPrompt = fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) + return true }() + + if !cont { + return + } } - currPrompt := fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) - shell.SetPrompt(currPrompt) - shell.SetMultiPrompt(fmt.Sprintf(fmt.Sprintf("%%%ds", len(currPrompt)), "-> ")) + shell.SetPrompt(nextPrompt) + shell.SetMultiPrompt(fmt.Sprintf(fmt.Sprintf("%%%ds", len(nextPrompt)), "-> ")) }) shell.Run() _ = iohelp.WriteLine(cli.CliOut, "Bye") - return returnedVerr + return nil } // Returns a new auto completer with table names, column names, and SQL keywords. @@ -1468,6 +1467,12 @@ func newSqlEngine( email := *dEnv.Config.GetStringOrDefault(env.UserEmailKey, "") sess, err := dsess.NewSession(sql.NewEmptyContext(), sql.NewBaseSession(), pro, username, email, dbStates...) + // TODO: this should just be the session default like it is with MySQL + err = sess.SetSessionVariable(sql.NewContext(ctx), sql.AutoCommitSessionVar, true) + if err != nil { + return nil, err + } + return &sqlEngine{ dbs: nameToDB, mrEnv: mrEnv, @@ -1602,17 +1607,7 @@ func (se *sqlEngine) getRoots(sqlCtx *sql.Context) (map[string]*doltdb.RootValue } func (se *sqlEngine) newContext(ctx context.Context) (*sql.Context, error) { - sqlCtx, err := se.contextFactory(ctx) - if err != nil { - return nil, err - } - - err = sqlCtx.SetSessionVariable(sqlCtx, sql.AutoCommitSessionVar, true) - if err != nil { - return nil, err - } - - return sqlCtx, nil + return se.contextFactory(ctx) } // Execute a SQL statement and return values for printing. From 8ae837f65975057fa0f68aeb187057b1c381016f Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 9 Sep 2021 14:18:23 -0700 Subject: [PATCH 07/12] Fixed bug in setting the prompt after a delimiter statement --- go/cmd/dolt/commands/sql.go | 61 +++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/go/cmd/dolt/commands/sql.go b/go/cmd/dolt/commands/sql.go index 20dc794d29..8441d66024 100644 --- a/go/cmd/dolt/commands/sql.go +++ b/go/cmd/dolt/commands/sql.go @@ -777,7 +777,6 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia currentDB := sqlCtx.Session.GetCurrentDatabase() currEnv := mrEnv[currentDB] - // start the doltsql shell historyFile := filepath.Join(".sqlhistory") // history file written to working dir initialPrompt := fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) initialMultilinePrompt := fmt.Sprintf(fmt.Sprintf("%%%ds", len(initialPrompt)), "-> ") @@ -840,52 +839,48 @@ func runShell(ctx context.Context, se *sqlEngine, mrEnv env.MultiRepoEnv, initia shell.Println(color.RedString(err.Error())) } - shouldProcessQuery := true //TODO: Handle comments and enforce the current line terminator if matches := delimiterRegex.FindStringSubmatch(query); len(matches) == 3 { // If we don't match from anything, then we just pass to the SQL engine and let it complain. shell.SetLineTerminator(matches[1]) - shouldProcessQuery = false + return } var nextPrompt string - if shouldProcessQuery { - var sqlSch sql.Schema - var rowIter sql.RowIter - var err error + var sqlSch sql.Schema + var rowIter sql.RowIter - // The SQL parser does not understand any other terminator besides semicolon, so we remove it. - if shell.LineTerminator() != ";" && strings.HasSuffix(query, shell.LineTerminator()) { - query = query[:len(query)-len(shell.LineTerminator())] + // The SQL parser does not understand any other terminator besides semicolon, so we remove it. + if shell.LineTerminator() != ";" && strings.HasSuffix(query, shell.LineTerminator()) { + query = query[:len(query)-len(shell.LineTerminator())] + } + + cont := func() bool { + subCtx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + defer stop() + + sqlCtx, err = se.newContext(subCtx) + if err != nil { + shell.Println(color.RedString(err.Error())) + return false } - cont := func() bool { - subCtx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) - defer stop() - - sqlCtx, err = se.newContext(subCtx) + if sqlSch, rowIter, err = processQuery(sqlCtx, query, se); err != nil { + verr := formatQueryError("", err) + shell.Println(verr.Verbose()) + } else if rowIter != nil { + err = PrettyPrintResults(sqlCtx, se.resultFormat, sqlSch, rowIter, HasTopLevelOrderByClause(query)) if err != nil { shell.Println(color.RedString(err.Error())) - return false } - - if sqlSch, rowIter, err = processQuery(sqlCtx, query, se); err != nil { - verr := formatQueryError("", err) - shell.Println(verr.Verbose()) - } else if rowIter != nil { - err = PrettyPrintResults(sqlCtx, se.resultFormat, sqlSch, rowIter, HasTopLevelOrderByClause(query)) - if err != nil { - shell.Println(color.RedString(err.Error())) - } - } - - nextPrompt = fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) - return true - }() - - if !cont { - return } + + nextPrompt = fmt.Sprintf("%s> ", sqlCtx.GetCurrentDatabase()) + return true + }() + + if !cont { + return } shell.SetPrompt(nextPrompt) From 4a5bb3cff2955d849ef40f325aff084a692ae906 Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Thu, 9 Sep 2021 14:19:36 -0700 Subject: [PATCH 08/12] go/cmd/dolt, go/utils/remotesrv: Fix using unbuffered channels with signal.Notify. --- go/cmd/dolt/dolt.go | 2 +- go/utils/remotesrv/main.go | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/go/cmd/dolt/dolt.go b/go/cmd/dolt/dolt.go index 5cc0c92ee9..4c46d7547b 100644 --- a/go/cmd/dolt/dolt.go +++ b/go/cmd/dolt/dolt.go @@ -227,7 +227,7 @@ func runMain() int { warnIfMaxFilesTooLow() ctx, cancelF := context.WithCancel(context.Background()) - c := make(chan os.Signal) + c := make(chan os.Signal, 1) signal.Notify(c, os.Interrupt, syscall.SIGTERM) go func() { <-c diff --git a/go/utils/remotesrv/main.go b/go/utils/remotesrv/main.go index 8d426810de..0127b5aae1 100644 --- a/go/utils/remotesrv/main.go +++ b/go/utils/remotesrv/main.go @@ -73,9 +73,8 @@ func main() { } func waitForSignal() { - c := make(chan os.Signal) - signal.Notify(c, os.Interrupt) - signal.Notify(c, os.Kill) + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt, os.Kill) <-c } From 841cce0ebc7cb7bbce4489169189238f817559a0 Mon Sep 17 00:00:00 2001 From: Daylon Wilkins Date: Thu, 9 Sep 2021 14:10:20 -0700 Subject: [PATCH 09/12] Bump Go version to 1.17 --- .github/workflows/cd-release.yaml | 2 +- .github/workflows/ci-bats-unix.yaml | 2 +- .github/workflows/ci-bats-windows.yaml | 2 +- .github/workflows/ci-check-repo.yaml | 2 +- .github/workflows/ci-compatibility-tests.yaml | 2 +- .github/workflows/ci-format-repo.yaml | 2 +- .github/workflows/ci-go-tests.yaml | 4 +-- go/go.mod | 29 ++++++++++++++++++- go/go.sum | 2 -- 9 files changed, 36 insertions(+), 11 deletions(-) diff --git a/.github/workflows/cd-release.yaml b/.github/workflows/cd-release.yaml index bd85004848..7e14f7a0ae 100644 --- a/.github/workflows/cd-release.yaml +++ b/.github/workflows/cd-release.yaml @@ -31,7 +31,7 @@ jobs: run: | latest=$(git rev-parse HEAD) echo "::set-output name=commitish::$latest" - GO_BUILD_VERSION=1.15.11 go/utils/publishrelease/buildbinaries.sh + GO_BUILD_VERSION=1.17.1 go/utils/publishrelease/buildbinaries.sh - name: Create Release id: create_release uses: actions/create-release@v1 diff --git a/.github/workflows/ci-bats-unix.yaml b/.github/workflows/ci-bats-unix.yaml index b3d5f904d2..7457b8091b 100644 --- a/.github/workflows/ci-bats-unix.yaml +++ b/.github/workflows/ci-bats-unix.yaml @@ -39,7 +39,7 @@ jobs: - name: Setup Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.15 + go-version: ^1.17 id: go - name: Setup Python 3.x uses: actions/setup-python@v2 diff --git a/.github/workflows/ci-bats-windows.yaml b/.github/workflows/ci-bats-windows.yaml index 3c1596e431..36a10f4e4d 100644 --- a/.github/workflows/ci-bats-windows.yaml +++ b/.github/workflows/ci-bats-windows.yaml @@ -84,7 +84,7 @@ jobs: - name: Setup Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.15 + go-version: ^1.17 id: go - name: Setup Python 3.x uses: actions/setup-python@v2 diff --git a/.github/workflows/ci-check-repo.yaml b/.github/workflows/ci-check-repo.yaml index b09066d1d9..027c4f014d 100644 --- a/.github/workflows/ci-check-repo.yaml +++ b/.github/workflows/ci-check-repo.yaml @@ -12,7 +12,7 @@ jobs: - name: Setup Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.15 + go-version: ^1.17 - uses: actions/checkout@v2 - name: Check all working-directory: ./go diff --git a/.github/workflows/ci-compatibility-tests.yaml b/.github/workflows/ci-compatibility-tests.yaml index bd99788945..2b90812a56 100644 --- a/.github/workflows/ci-compatibility-tests.yaml +++ b/.github/workflows/ci-compatibility-tests.yaml @@ -16,7 +16,7 @@ jobs: - name: Setup Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.13 + go-version: ^1.17 id: go - uses: actions/checkout@v2 - uses: actions/setup-node@v1 diff --git a/.github/workflows/ci-format-repo.yaml b/.github/workflows/ci-format-repo.yaml index 036e1d6166..f0b319bdd2 100644 --- a/.github/workflows/ci-format-repo.yaml +++ b/.github/workflows/ci-format-repo.yaml @@ -12,7 +12,7 @@ jobs: - name: Setup Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.15 + go-version: ^1.17 - uses: actions/checkout@v2 with: token: ${{ secrets.REPO_ACCESS_TOKEN || secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/ci-go-tests.yaml b/.github/workflows/ci-go-tests.yaml index 4e02246ac4..9e1f17df0f 100644 --- a/.github/workflows/ci-go-tests.yaml +++ b/.github/workflows/ci-go-tests.yaml @@ -22,7 +22,7 @@ jobs: - name: Set up Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.15 + go-version: ^1.17 id: go - uses: actions/checkout@v2 - name: Test All @@ -69,7 +69,7 @@ jobs: - name: Set up Go 1.x uses: actions/setup-go@v2 with: - go-version: ^1.15 + go-version: ^1.17 id: go - uses: actions/checkout@v2 - name: Test All diff --git a/go/go.mod b/go/go.mod index df263338dc..25a866dc1c 100644 --- a/go/go.mod +++ b/go/go.mod @@ -1,12 +1,14 @@ module github.com/dolthub/dolt/go require ( + cloud.google.com/go v0.66.0 // indirect cloud.google.com/go/storage v1.12.0 github.com/BurntSushi/toml v0.3.1 github.com/HdrHistogram/hdrhistogram-go v1.0.0 github.com/StackExchange/wmi v0.0.0-20190523213315-cbe66965904d // indirect github.com/abiosoft/readline v0.0.0-20180607040430-155bce2042db github.com/acarl005/stripansi v0.0.0-20180116102854-5a71ef0e047d + github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751 // indirect github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d // indirect github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535 // indirect @@ -15,7 +17,9 @@ require ( github.com/bcicen/jstream v1.0.0 github.com/boltdb/bolt v1.3.1 github.com/cenkalti/backoff v2.2.1+incompatible + github.com/cespare/xxhash v1.1.0 // indirect github.com/codahale/blake2 v0.0.0-20150924215134-8d10d0420cbf + github.com/davecgh/go-spew v1.1.1 // indirect github.com/denisbrodbeck/machineid v1.0.1 github.com/dolthub/dolt/go/gen/proto/dolt/services/eventsapi v0.0.0-20201005193433-3ee972b1d078 github.com/dolthub/fslock v0.0.3 @@ -28,29 +32,39 @@ require ( github.com/fatih/color v1.9.0 github.com/flynn-archive/go-shlex v0.0.0-20150515145356-3f9db97f8568 github.com/go-kit/kit v0.10.0 // indirect + github.com/go-ole/go-ole v1.2.1 // indirect github.com/go-openapi/errors v0.19.6 // indirect github.com/go-openapi/strfmt v0.19.5 // indirect github.com/go-sql-driver/mysql v1.6.0 + github.com/go-stack/stack v1.8.0 // indirect github.com/gocraft/dbr/v2 v2.7.0 github.com/golang/glog v0.0.0-20210429001901-424d2337a529 // indirect + github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect github.com/golang/protobuf v1.5.2 github.com/golang/snappy v0.0.1 github.com/google/go-cmp v0.5.5 github.com/google/uuid v1.2.0 + github.com/googleapis/gax-go/v2 v2.0.5 // indirect github.com/hashicorp/golang-lru v0.5.4 // indirect + github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/jedib0t/go-pretty v4.3.1-0.20191104025401-85fe5d6a7c4d+incompatible + github.com/jmespath/go-jmespath v0.3.0 // indirect github.com/jpillora/backoff v1.0.0 + github.com/jstemmer/go-junit-report v0.9.1 // indirect github.com/juju/gnuflag v0.0.0-20171113085948-2ce1bb71843d github.com/kch42/buzhash v0.0.0-20160816060738-9bdec3dec7c6 github.com/lestrrat-go/strftime v1.0.4 // indirect + github.com/mattn/go-colorable v0.1.7 // indirect github.com/mattn/go-isatty v0.0.12 github.com/mattn/go-runewidth v0.0.9 github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b github.com/mitchellh/hashstructure v1.1.0 // indirect github.com/mitchellh/mapstructure v1.3.2 // indirect + github.com/oliveagle/jsonpath v0.0.0-20180606110733-2e52cf6e6852 // indirect github.com/opentracing/opentracing-go v1.2.0 github.com/pkg/errors v0.9.1 github.com/pkg/profile v1.5.0 + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/rivo/uniseg v0.1.0 github.com/sergi/go-diff v1.1.0 // indirect github.com/shirou/gopsutil v3.21.2+incompatible @@ -59,19 +73,32 @@ require ( github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966 github.com/spf13/cast v1.3.1 // indirect github.com/spf13/cobra v1.0.0 + github.com/spf13/pflag v1.0.5 // indirect + github.com/src-d/go-oniguruma v1.1.0 // indirect github.com/stretchr/testify v1.7.0 github.com/tealeg/xlsx v1.0.5 github.com/tidwall/pretty v1.0.1 // indirect github.com/tklauser/go-sysconf v0.3.5 // indirect + github.com/tklauser/numcpus v0.2.2 // indirect github.com/uber/jaeger-client-go v2.25.0+incompatible github.com/uber/jaeger-lib v2.4.0+incompatible // indirect go.mongodb.org/mongo-driver v1.7.0 // indirect + go.opencensus.io v0.22.4 // indirect + go.uber.org/atomic v1.6.0 // indirect + go.uber.org/multierr v1.5.0 // indirect go.uber.org/zap v1.15.0 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 + golang.org/x/lint v0.0.0-20201208152925-83fdc39ff7b5 // indirect + golang.org/x/mod v0.3.0 // indirect golang.org/x/net v0.0.0-20210505214959-0714010a04ed + golang.org/x/oauth2 v0.0.0-20200902213428-5d25da1a8d43 // indirect golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9 golang.org/x/sys v0.0.0-20210503173754-0981d6026fa6 + golang.org/x/text v0.3.6 // indirect + golang.org/x/tools v0.1.0 // indirect + golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect google.golang.org/api v0.32.0 + google.golang.org/appengine v1.6.7 // indirect google.golang.org/genproto v0.0.0-20210506142907-4a47615972c2 // indirect google.golang.org/grpc v1.37.0 google.golang.org/protobuf v1.26.0 @@ -83,4 +110,4 @@ require ( replace github.com/dolthub/dolt/go/gen/proto/dolt/services/eventsapi => ./gen/proto/dolt/services/eventsapi -go 1.15 +go 1.17 diff --git a/go/go.sum b/go/go.sum index aa2e0e529d..2c51d05af3 100644 --- a/go/go.sum +++ b/go/go.sum @@ -144,8 +144,6 @@ github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZm github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no= github.com/dolthub/fslock v0.0.3 h1:iLMpUIvJKMKm92+N1fmHVdxJP5NdyDK5bK7z7Ba2s2U= github.com/dolthub/fslock v0.0.3/go.mod h1:QWql+P17oAAMLnL4HGB5tiovtDuAjdDTPbuqx7bYfa0= -github.com/dolthub/go-mysql-server v0.10.1-0.20210902175808-c05838e8d173 h1:gM1YsiCfQMVcaSQrj9ZZMvJStGi0/TZc7zwsf9KbweI= -github.com/dolthub/go-mysql-server v0.10.1-0.20210902175808-c05838e8d173/go.mod h1:cPg39xeFH8/+McnJxncb79SgUuREeIqR+eTvxE6OmXc= github.com/dolthub/go-mysql-server v0.10.1-0.20210903190613-4c25c32c3883 h1:IfpwAn8PtCr1roskOckNiNxj2Iqly0yTtWeWkgla0YM= github.com/dolthub/go-mysql-server v0.10.1-0.20210903190613-4c25c32c3883/go.mod h1:cPg39xeFH8/+McnJxncb79SgUuREeIqR+eTvxE6OmXc= github.com/dolthub/ishell v0.0.0-20210205014355-16a4ce758446 h1:0ol5pj+QlKUKAtqs1LiPM3ZJKs+rHPgLSsMXmhTrCAM= From 474c7aaee292843d2ff78713526ff1537867020a Mon Sep 17 00:00:00 2001 From: Abdurrahmaan Iqbal Date: Wed, 8 Sep 2021 22:41:02 +0100 Subject: [PATCH 10/12] Modify bad row error format --- go/libraries/doltcore/row/row.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/libraries/doltcore/row/row.go b/go/libraries/doltcore/row/row.go index df053e92b6..175c4091be 100644 --- a/go/libraries/doltcore/row/row.go +++ b/go/libraries/doltcore/row/row.go @@ -201,7 +201,7 @@ func findInvalidCol(r Row, sch schema.Schema) (*schema.Column, schema.ColConstra if !col.TypeInfo.IsValid(val) { badCol = &col - return true, fmt.Errorf(`"%v" is not valid for "%v"`, val, col.TypeInfo.String()) + return true, fmt.Errorf(`"%v" is not valid for column "%s" (type "%s")`, val, col.Name, col.TypeInfo.String()) } if len(col.Constraints) > 0 { From a11b64e69260a059f7bedcbc165e7c70035d4d52 Mon Sep 17 00:00:00 2001 From: Abdurrahmaan Iqbal Date: Thu, 9 Sep 2021 20:28:26 +0100 Subject: [PATCH 11/12] Modify error message format and add test --- go/libraries/doltcore/row/row.go | 2 +- .../bats/import-update-tables.bats | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/go/libraries/doltcore/row/row.go b/go/libraries/doltcore/row/row.go index 175c4091be..1ffac5d745 100644 --- a/go/libraries/doltcore/row/row.go +++ b/go/libraries/doltcore/row/row.go @@ -201,7 +201,7 @@ func findInvalidCol(r Row, sch schema.Schema) (*schema.Column, schema.ColConstra if !col.TypeInfo.IsValid(val) { badCol = &col - return true, fmt.Errorf(`"%v" is not valid for column "%s" (type "%s")`, val, col.Name, col.TypeInfo.String()) + return true, fmt.Errorf(`"%v" is not valid for column "%s" (type "%s")`, val, col.Name, col.TypeInfo.ToSqlType().String()) } if len(col.Constraints) > 0 { diff --git a/integration-tests/bats/import-update-tables.bats b/integration-tests/bats/import-update-tables.bats index bedbc767cb..73bce7f3a9 100644 --- a/integration-tests/bats/import-update-tables.bats +++ b/integration-tests/bats/import-update-tables.bats @@ -14,6 +14,14 @@ CREATE TABLE test ( c5 BIGINT COMMENT 'tag:5', PRIMARY KEY (pk) ); +SQL + + cat < 1pk1col-char-sch.sql +CREATE TABLE test ( + pk BIGINT NOT NULL COMMENT 'tag:0', + c char(5) COMMENT 'tag:1', + PRIMARY KEY (pk) +); SQL cat < 1pk5col-ints.csv @@ -186,6 +194,20 @@ SQL [[ "${lines[6]}" =~ "end date" ]] || false } +@test "import-update-tables: update table with incorrect length char throws bad row error" { + cat < 1pk1col-rpt-chars.csv +pk,c +1,"123456" +DELIM + + dolt sql < 1pk1col-char-sch.sql + run dolt table import -u test 1pk1col-rpt-chars.csv + [ "$status" -eq 1 ] + [[ "$output" =~ "A bad row was encountered while moving data" ]] || false + [[ "$output" =~ "Bad Row:" ]] || false + [[ "$output" =~ '"123456" is not valid for column "c" (type "CHAR(5)")' ]] || false +} + @test "import-update-tables: update table with repeat pk in csv throws error" { cat < 1pk5col-rpt-ints.csv pk,c1,c2,c3,c4,c5 From e17bfddf572f3ca9faf05155790258cc24ad16c0 Mon Sep 17 00:00:00 2001 From: Abdurrahmaan Iqbal Date: Thu, 9 Sep 2021 21:13:49 +0100 Subject: [PATCH 12/12] All-caps CHAR for consistency --- integration-tests/bats/import-update-tables.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/bats/import-update-tables.bats b/integration-tests/bats/import-update-tables.bats index 73bce7f3a9..9ee251e43b 100644 --- a/integration-tests/bats/import-update-tables.bats +++ b/integration-tests/bats/import-update-tables.bats @@ -19,7 +19,7 @@ SQL cat < 1pk1col-char-sch.sql CREATE TABLE test ( pk BIGINT NOT NULL COMMENT 'tag:0', - c char(5) COMMENT 'tag:1', + c CHAR(5) COMMENT 'tag:1', PRIMARY KEY (pk) ); SQL