Merge pull request #6857 from dolthub/steph/import-error

Improve import table error message
dolthub · Oct 21, 2023 · 41a47d9 · 41a47d9
2 parents 029fa6c + a02ee53
commit 41a47d9
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 22 deletions.
diff --git a/go/cmd/dolt/commands/tblcmds/import.go b/go/cmd/dolt/commands/tblcmds/import.go
@@ -389,10 +389,9 @@ func (cmd ImportCmd) Exec(ctx context.Context, commandStr string, args []string,
 
 	help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, importDocs, ap))
 	apr := cli.ParseArgsOrDie(ap, args, help)
+	var verr errhand.VerboseError
 
 	dEnv, err := commands.MaybeMigrateEnv(ctx, dEnv)
-
-	var verr errhand.VerboseError
 	if err != nil {
 		verr = errhand.BuildDError("could not load manifest for gc").AddCause(err).Build()
 		return commands.HandleVErrAndExitCode(verr, usage)
@@ -404,13 +403,11 @@ func (cmd ImportCmd) Exec(ctx context.Context, commandStr string, args []string,
 	}
 
 	mvOpts, verr := getImportMoveOptions(ctx, apr, dEnv)
-
 	if verr != nil {
 		return commands.HandleVErrAndExitCode(verr, usage)
 	}
 
 	root, err := dEnv.WorkingRoot(ctx)
-
 	if err != nil {
 		verr = errhand.BuildDError("Unable to get the working root value for this data repository.").AddCause(err).Build()
 		return commands.HandleVErrAndExitCode(verr, usage)
@@ -432,9 +429,7 @@ func (cmd ImportCmd) Exec(ctx context.Context, commandStr string, args []string,
 	if err != nil {
 		bdr := errhand.BuildDError("\nAn error occurred while moving data")
 		bdr.AddCause(err)
-
 		bdr.AddDetails("Errors during import can be ignored using '--continue'")
-
 		return commands.HandleVErrAndExitCode(bdr.Build(), usage)
 	}
 
@@ -518,7 +513,7 @@ func newImportSqlEngineMover(ctx context.Context, dEnv *env.DoltEnv, rdSchema sc
 	return mv, nil
 }
 
-type badRowFn func(row sql.Row, err error) (quit bool)
+type badRowFn func(row sql.Row, rowSchema sql.PrimaryKeySchema, tableName string, lineNumber int, err error) (quit bool)
 
 func move(ctx context.Context, rd table.SqlRowReader, wr *mvdata.SqlEngineTableWriter, options *importOptions) (int64, error) {
 	g, ctx := errgroup.WithContext(ctx)
@@ -529,10 +524,20 @@ func move(ctx context.Context, rd table.SqlRowReader, wr *mvdata.SqlEngineTableW
 	var printBadRowsStarted bool
 	var badCount int64
 
-	badRowCB := func(row sql.Row, err error) (quit bool) {
+	badRowCB := func(row sql.Row, rowSchema sql.PrimaryKeySchema, tableName string, lineNumber int, err error) (quit bool) {
 		// record the first error encountered unless asked to ignore it
 		if row != nil && rowErr == nil && !options.contOnErr {
-			rowErr = fmt.Errorf("A bad row was encountered: %s: %w", sql.FormatRow(row), err)
+			var sqlRowWithColumns []string
+			for i, val := range row {
+				columnName := "<nil>"
+				if len(rowSchema.Schema) > i {
+					columnName = rowSchema.Schema[i].Name
+				}
+				sqlRowWithColumns = append(sqlRowWithColumns, fmt.Sprintf("\t%s: %v\n", columnName, val))
+			}
+			formattedSqlRow := strings.Join(sqlRowWithColumns, "")
+
+			rowErr = fmt.Errorf("A bad row was encountered inserting into table %s (on line %d):\n%s", tableName, lineNumber, formattedSqlRow)
 			if wie, ok := err.(sql.WrappedInsertError); ok {
 				if e, ok := wie.Cause.(*errors.Error); ok {
 					if ue, ok := e.Cause().(sql.UniqueKeyError); ok {
@@ -615,15 +620,18 @@ func moveRows(
 		return err
 	}
 
+	line := 1
+
 	for {
 		sqlRow, err := rd.ReadSqlRow(ctx)
 		if err == io.EOF {
 			return nil
 		}
+		line += 1
 
 		if err != nil {
 			if table.IsBadRow(err) {
-				quit := badRowCb(sqlRow, err)
+				quit := badRowCb(sqlRow, rdSqlSch, options.destTableName, line, err)
 				if quit {
 					return err
 				}

diff --git a/go/libraries/doltcore/mvdata/engine_table_writer.go b/go/libraries/doltcore/mvdata/engine_table_writer.go
@@ -128,7 +128,7 @@ func NewSqlEngineTableWriter(ctx context.Context, dEnv *env.DoltEnv, createTable
 	}, nil
 }
 
-func (s *SqlEngineTableWriter) WriteRows(ctx context.Context, inputChannel chan sql.Row, badRowCb func(row sql.Row, err error) bool) (err error) {
+func (s *SqlEngineTableWriter) WriteRows(ctx context.Context, inputChannel chan sql.Row, badRowCb func(row sql.Row, rowSchema sql.PrimaryKeySchema, tableName string, lineNumber int, err error) bool) (err error) {
 	err = s.forceDropTableIfNeeded()
 	if err != nil {
 		return err
@@ -190,13 +190,16 @@ func (s *SqlEngineTableWriter) WriteRows(ctx context.Context, inputChannel chan
 		}
 	}()
 
+	line := 1
+
 	for {
 		if s.statsCB != nil && atomic.LoadInt32(&s.statOps) >= tableWriterStatUpdateRate {
 			atomic.StoreInt32(&s.statOps, 0)
 			s.statsCB(s.stats)
 		}
 
 		row, err := iter.Next(s.sqlCtx)
+		line += 1
 
 		// All other errors are handled by the errorHandler
 		if err == nil {
@@ -219,7 +222,7 @@ func (s *SqlEngineTableWriter) WriteRows(ctx context.Context, inputChannel chan
 				offendingRow = n.OffendingRow
 			}
 
-			quit := badRowCb(offendingRow, err)
+			quit := badRowCb(offendingRow, s.tableSchema, s.tableName, line, err)
 			if quit {
 				return err
 			}

diff --git a/integration-tests/bats/default-values.bats b/integration-tests/bats/default-values.bats
@@ -486,7 +486,9 @@ DELIM
     run dolt table import -u test bad-update.csv
     [ "$status" -eq "1" ]
     [[ "$output" =~ "bad row" ]] || false
-    [[ "$output" =~ "[5,<nil>,5]" ]] || false
+    [[ "$output" =~ "pk: 5" ]] || false
+    [[ "$output" =~ "v1: <nil>" ]] || false
+    [[ "$output" =~ "v2: 5" ]] || false
     [[ "$output" =~ "column name 'v1' is non-nullable but attempted to set a value of null" ]] || false
 }
 

diff --git a/integration-tests/bats/import-append-tables.bats b/integration-tests/bats/import-append-tables.bats
@@ -26,7 +26,6 @@ CSV
     [[ "$output" =~ "row [1,1] would be overwritten by [1,2]" ]] || false
 
     run dolt sql -q "select * from t"
-    echo "$output"
     [ "$status" -eq 0 ]
     [[   "$output" =~ "| 1  | 1    |" ]] || false
     [[ ! "$output" =~ "| 1  | 2    |" ]] || false
@@ -41,7 +40,6 @@ pk, col1
 1, 2
 CSV
 
-    echo "$output"
     [ "$status" -eq 1 ]
     [[ "$output" =~ "An error occurred while moving data" ]] || false
     [[ "$output" =~ "row [1,1] would be overwritten by [1,2]" ]] || false
@@ -102,4 +100,21 @@ CSV
     [[   "$output" =~ "| 2  | 3    |" ]] || false
     [[ ! "$output" =~ "| 1  | 2    |" ]] || false
     [ "${#lines[@]}" -eq 6 ]
-}
+}
+
+@test "import-append-tables: import error message contains useful information" {
+    dolt sql -q "CREATE TABLE shirts (name VARCHAR(40), size ENUM('x-small', 'small', 'medium', 'large', 'x-large'), color ENUM('red', 'blue'));"
+    run dolt table import -a shirts <<CSV
+name, size, color
+"shirt1", "x-small", "red"
+"shirt2", "other", "green"
+CSV
+    [ "$status" -eq 1 ]
+    [[ "$output" =~ "An error occurred while moving data" ]] || false
+    [[ "$output" =~ "cause: value other is not valid for this Enum" ]] || false
+    [[ "$output" =~ "A bad row was encountered inserting into table shirts (on line 3):" ]] || false    # table name
+    [[ "$output" =~ "name: shirt2" ]] || false                                              # column names
+    [[ "$output" =~ "size: other" ]] || false
+    [[ "$output" =~ "color: green" ]] || false
+    [[ "$output" =~ "Errors during import can be ignored using '--continue'" ]] || false
+}
diff --git a/integration-tests/bats/import-update-tables.bats b/integration-tests/bats/import-update-tables.bats
@@ -281,7 +281,8 @@ DELIM
     [ "$status" -eq 1 ]
     [[ "$output" =~ "An error occurred while moving data" ]] || false
     [[ "$output" =~ "A bad row was encountered" ]] || false
-    [[ "$output" =~ "[1,123456]" ]] || false    
+    [[ "$output" =~ "pk: 1" ]] || false
+    [[ "$output" =~ "c: 123456" ]] || false
     [[ "$output" =~ 'too large for column' ]] || false
 }
 
@@ -314,9 +315,11 @@ DELIM
     dolt sql < check-constraint-sch.sql
     run dolt table import -u persons persons.csv
     [ "$status" -eq 1 ]
-
     [[ "$output" =~ "A bad row was encountered" ]] || false
-    [[ "$output" =~ "[2,little,doe,10]" ]] || false
+    [[ "$output" =~ "ID: 2" ]] || false
+    [[ "$output" =~ "LastName: little" ]] || false
+    [[ "$output" =~ "FirstName: doe" ]] || false
+    [[ "$output" =~ "Age: 10" ]] || false
 
     run dolt table import -u --continue persons persons.csv
     [ "$status" -eq 0 ]
@@ -404,7 +407,7 @@ DELIM
     run dolt table import -u test bad-updates.csv
     [ "$status" -eq 1 ]
     [[ "$output" =~ "A bad row was encountered" ]] || false
-    [[ "$output" =~ "[100]" ]] || false
+    [[ "$output" =~ "pk: 100" ]] || false
 
     run dolt table import -u --continue test bad-updates.csv
     [ "$status" -eq 0 ]
@@ -806,7 +809,9 @@ DELIM
     run dolt table import -u objects objects-bad.csv
     [ "$status" -eq 1 ]
     [[ "$output" =~ "A bad row was encountered" ]] || false
-    [[ "$output" =~ "[6,bottle,gray]" ]] || false
+    [[ "$output" =~ "id: 6" ]] || false
+    [[ "$output" =~ "name: bottle" ]] || false
+    [[ "$output" =~ "color: gray" ]] || false
     [[ "$output" =~ "cannot add or update a child row - Foreign key violation" ]] || false
 
     run dolt table import -u objects objects-bad.csv --continue
@@ -886,7 +891,10 @@ DELIM
     run dolt table import -u objects multi-key-bad.csv
     [ "$status" -eq 1 ]
     [[ "$output" =~ "A bad row was encountered" ]] || false
-    [[ "$output" =~ "[6,bottle,blue,steel]" ]] || false
+    [[ "$output" =~ "id: 6" ]] || false
+    [[ "$output" =~ "name: bottle" ]] || false
+    [[ "$output" =~ "color: blue" ]] || false
+    [[ "$output" =~ "material: steel" ]] || false
     [[ "$output" =~ "cannot add or update a child row - Foreign key violation" ]] || false
 
     run dolt table import -u objects multi-key-bad.csv --continue