importccl: Support mysql's OUTFILE format in IMPORT #25615

dt · 2018-05-17T15:07:04Z

This adds support for MySQL's OUTFILE (as created by mysqldump --tab) to IMPORT.

Without this field it falls back to the legacy opts which assume CSV only. Release note: none.

cockroach-teamcity · 2018-05-17T15:07:12Z

This change is

maddyblue · 2018-05-17T17:45:40Z

pkg/ccl/importccl/read_import_proc.go

@@ -337,6 +337,12 @@ func newReadImportDataProcessor(
 	return cp, nil
 }

+type inputConverter interface {
+	start(ctx context.Context, group *errgroup.Group)
+	readFile(context.Context, io.Reader, int32, string, func(finished bool) error) error


all of these arguments should be named

maddyblue · 2018-05-17T17:48:56Z

pkg/ccl/importccl/read_import_mysqlout_test.go

+
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	_ "github.com/go-sql-driver/mysql"


this requires us to vendor. is there a way around doing that? like moving writeMysqlOutfileTestdata to a comment or a file with a build ignore tag? not sure. or should we just vendor it?

We already vendor it: https://github.com/cockroachdb/cockroach/blob/master/Gopkg.lock#L413

maddyblue · 2018-05-17T17:56:11Z

pkg/ccl/importccl/read_import_mysqlout.go

+	input io.Reader,
+	inputIdx int32,
+	inputName string,
+	progressFn func(finished bool) error,


this type is used a bunch now and maybe should get its own type? don't care too much.

maddyblue · 2018-05-17T18:04:01Z

pkg/ccl/importccl/read_import_mysqlout.go

+		}
+
+		// If we have a defined enclose char, check if we're starting or stopping
+		// an enclosed field. Technically, the enclose char can be made mandatory,


Is the flag described in this comment d.opts.Enclose? If so fix this comment.

maddyblue · 2018-05-17T18:05:20Z

pkg/ccl/importccl/read_import_mysqlout.go

+		// so potentially we could have a flag to enforce that, returning an error
+		// if it is missing, but we don't need to care for simply decoding.
+		if d.opts.Enclose != roachpb.MySQLOutfileOptions_Never && c == d.opts.Encloser {
+			readingField = !readingField


if d.opts.Encloser appears multiple times in a row they will just keep flipping readingField. Is that intended and correct?

maddyblue · 2018-05-17T18:09:09Z

pkg/ccl/importccl/read_import_mysqlout_test.go

@@ -0,0 +1,312 @@
+// Copyright 2018 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");


Should this be CCL?

maddyblue · 2018-05-17T18:14:40Z

pkg/ccl/importccl/read_import_mysqlout_test.go

+		{
+			name: "escape-and-enclose",
+			opts: roachpb.MySQLOutfileOptions{
+				FieldSeparator: '\t', RowSeparator: '\n', HasEscape: true, Escape: '\\', Enclose: roachpb.MySQLOutfileOptions_Always, Encloser: '"',


these would be easier to read separated by newlines

maddyblue · 2018-05-17T18:15:40Z

pkg/ccl/importccl/read_import_mysqlout_test.go

+	ctx := context.TODO()
+	for _, config := range configs {
+		t.Run(config.name, func(t *testing.T) {
+			converter := newMysqloutfileReader(ctx, nil, config.opts, nil, 3)


could this 3 be a named const?

maddyblue · 2018-05-17T18:20:17Z

pkg/ccl/importccl/read_import_mysqlout_test.go

+					// If the csv writing part of our conversion encodes as hex, we can
+					// round-trip bytes though the go csv parser.
+					orig := r[2]
+					r[2] = hex.EncodeToString([]byte(r[2]))


i don't understand what's going on here. why don't we have to append \x to this?

maddyblue · 2018-05-17T18:39:46Z

pkg/ccl/importccl/read_import_mysqlout_test.go

+					expected = []byte(config.null)
+				}
+
+				actual := []byte(res[i][2])


i don't see where res[i][0] is verified

maddyblue · 2018-05-17T19:34:34Z

pkg/ccl/importccl/import_stmt.go

+			return errors.Errorf("unsupported import format: %q", importStmt.FileFormat)
+		}
+
+		if format.Format != roachpb.IOFileFormat_CSV {


Is it better to move these checks to be per format instead of anything not CSV? When we add mysql SQL support we'll need another version bump and we'll have to split it apart there again.

maddyblue · 2018-05-17T19:36:08Z

pkg/ccl/importccl/import_stmt_test.go

+				}
+
+				if expected != actual {
+					t.Fatalf("expected rowi=%s string to be %q, got %q", row[0], expected, actual)


spelling? rowi

maddyblue · 2018-05-17T19:43:08Z

pkg/util/strings.go

 	if r == utf8.RuneError {
 		return 0, errors.Errorf("invalid character: %s", s)
 	}
-	if sz != len(s) {
-		return r, errors.New("must be only one character")
+	if l := len(s); 1 != l {


I think it was correct before. Add a descriptive comment if this is indeed intended. The length of a string is the number of bytes, not the number of runes. See https://play.golang.org/p/SE0ybC7jWoN

dt · 2018-05-18T04:37:24Z

Review status: 0 of 21 files reviewed at latest revision, 14 unresolved discussions.

pkg/ccl/importccl/import_stmt.go, line 393 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Is it better to move these checks to be per format instead of anything not CSV? When we add mysql SQL support we'll need another version bump and we'll have to split it apart there again.

I think we just need to check for not CSV: if all the nodes are aware of the IOFormat proto, they'll know to look at the enum, and correctly fail if they don't support a format. I added a better error for that in the processor.

pkg/ccl/importccl/import_stmt_test.go, line 1181 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

spelling? rowi

Done.

pkg/ccl/importccl/read_import_mysqlout.go, line 47 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

this type is used a bunch now and maybe should get its own type? don't care too much.

Done.

pkg/ccl/importccl/read_import_mysqlout.go, line 147 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Is the flag described in this comment d.opts.Enclose? If so fix this comment.

Done.

pkg/ccl/importccl/read_import_mysqlout.go, line 151 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

if d.opts.Encloser appears multiple times in a row they will just keep flipping readingField. Is that intended and correct?

Eh, that's more or less my read of the docs. They say it has to be escaped if it occurs in the content.

pkg/ccl/importccl/read_import_mysqlout.go, line 162 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

there's a nice error message here, but not in any other error return path. i think we should make a general solution (like a struct with an Error method) to not introduce more things as described in #25532. doesn't need to be done in this pr.

hm, there aren't actually many error returns mid-file -- I think it is just this one and then io errors from reading bytes from the reader. There are two error returns that can happen at end-of-file.

pkg/ccl/importccl/read_import_mysqlout_test.go, line 3 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Should this be CCL?

Done.

pkg/ccl/importccl/read_import_mysqlout_test.go, line 170 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

these would be easier to read separated by newlines

Done.

pkg/ccl/importccl/read_import_mysqlout_test.go, line 211 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

could this 3 be a named const?

Done.

pkg/ccl/importccl/read_import_mysqlout_test.go, line 295 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

i don't see where res[i][0] is verified

res[i][0] wasn't explicitly verified, was just being used as a label in case the other cols didn't line up, but I guess I can verify it too.

pkg/ccl/importccl/read_import_proc.go, line 342 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

all of these arguments should be named

Done.

pkg/util/strings.go, line 32 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I think it was correct before. Add a descriptive comment if this is indeed intended. The length of a string is the number of bytes, not the number of runes. See https://play.golang.org/p/SE0ybC7jWoN

Done.

Comments from Reviewable

maddyblue · 2018-05-18T07:40:57Z

Review status: 0 of 21 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/ccl/importccl/read_import_mysqlout.go, line 162 at r2 (raw file):

Previously, dt (David Taylor) wrote…

hm, there aren't actually many error returns mid-file -- I think it is just this one and then io errors from reading bytes from the reader. There are two error returns that can happen at end-of-file.

i'm specifically worried about the finished block above where it checks for expected enclosure chars and stuff. in any case can happen later.

pkg/ccl/importccl/read_import_mysqlout_test.go, line 34 at r2 (raw file):

Previously, dt (David Taylor) wrote…

We already vendor it: https://github.com/cockroachdb/cockroach/blob/master/Gopkg.lock#L413

TIL

pkg/ccl/importccl/read_import_mysqlout_test.go, line 245 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

i don't understand what's going on here. why don't we have to append \x to this?

reminder about this question

Comments from Reviewable

dt · 2018-05-21T17:40:54Z

Review status: 0 of 21 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

pkg/ccl/importccl/read_import_mysqlout_test.go, line 245 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

reminder about this question

This is writing to a CSV, and later we read that CSV.

This was particularly useful when we were testing mysqlout->csv->cockroach. Now that we're doing mysqlout->cockroach, I'm not sure if we need to read and write csvs at all -- I kept them though since they're more human readable than the raw mysqlout.

Comments from Reviewable

Release note: none.

Release note (sql change): Add support for MySQL's tabbed OUTFILE format to IMPORT.

dt · 2018-05-21T19:53:03Z

bors r=mjibson

25615: importccl: Support mysql's OUTFILE format in IMPORT r=mjibson a=dt This adds support for MySQL's OUTFILE (as created by `mysqldump --tab`) to IMPORT. Co-authored-by: David Taylor <[email protected]>

craig · 2018-05-21T20:12:28Z

Build succeeded

GitHub CI (Cockroach)

importccl: actually pass correct format config to import job

6062937

Without this field it falls back to the legacy opts which assume CSV only. Release note: none.

dt requested review from maddyblue, bobvawter and a team May 17, 2018 15:07

dt force-pushed the import-mysqlout branch 2 times, most recently from fcece6a to 80195e6 Compare May 17, 2018 17:06

dt mentioned this pull request May 17, 2018

cmd/mysqlout: add utility for converting mysql OUTFILE to CSV #25287

Closed

dt force-pushed the import-mysqlout branch from 80195e6 to e88994b Compare May 17, 2018 17:30

maddyblue reviewed May 17, 2018

View reviewed changes

dt force-pushed the import-mysqlout branch from e88994b to 30a1607 Compare May 18, 2018 04:35

dt added 2 commits May 21, 2018 17:46

importccl: add mysqloutfile converter

09bba2d

Release note: none.

importccl: hook up MYSQLOUTFILE as a valid IMPORT format

b24d85e

Release note (sql change): Add support for MySQL's tabbed OUTFILE format to IMPORT.

dt force-pushed the import-mysqlout branch from 30a1607 to b24d85e Compare May 21, 2018 17:47

dt requested a review from a team May 21, 2018 17:47

maddyblue approved these changes May 21, 2018

View reviewed changes

craig bot merged commit b24d85e into cockroachdb:master May 21, 2018

jseldess mentioned this pull request May 30, 2018

importccl: Support mysql's OUTFILE format in IMPORT cockroachdb/docs#3218

Closed

dt deleted the import-mysqlout branch June 5, 2018 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importccl: Support mysql's OUTFILE format in IMPORT #25615

importccl: Support mysql's OUTFILE format in IMPORT #25615

dt commented May 17, 2018

cockroach-teamcity commented May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

dt May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

maddyblue May 17, 2018

dt commented May 18, 2018

maddyblue commented May 18, 2018

dt commented May 21, 2018

dt commented May 21, 2018

craig bot commented May 21, 2018

importccl: Support mysql's OUTFILE format in IMPORT #25615

importccl: Support mysql's OUTFILE format in IMPORT #25615

Conversation

dt commented May 17, 2018

cockroach-teamcity commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dt commented May 18, 2018

maddyblue commented May 18, 2018

dt commented May 21, 2018

dt commented May 21, 2018

craig bot commented May 21, 2018

Build succeeded