Add support for executing SQL statements from the command line.

[knz]

With this patch the user can use "sql -e ..." to execute statements,
print their results and exit, without an interactive prompt.

Fixes #3817

[RaduBerinde]

LGTM

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[petermattis]

Nice, be the change you're looking for.

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

cli/cli_test.go, line 475 [r1] (raw file):
Did you intend for there to be separate arguments for select 3 and select * from t.f? How is that working? Ah, you allow extra positional arguments as queries as well. Hmm, I guess that's convenient.

---

cli/cli_test.go, line 483 [r1] (raw file):
The incorrect pluralization hurts my eyes.

---

cli/flags.go, line 156 [r1] (raw file):
Is the indentation correct here? It looks off in reviewable.

---

cli/flags.go, line 264 [r1] (raw file):
The OneShotSQL flag is only used within the cli package. There is no need to place it within server.Context. I think it is time to introduce a cli.context type:

type Context struct {
  *server.Context
  OneShotSQL bool
}

var context Context = { Context:server.NewContext }

---

cli/sql.go, line 142 [r1] (raw file):
Our style is for comments to have proper capitalization and punctuation.

---

Comments from the review on Reviewable.io

[knz]

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

cli/cli_test.go, line 475 [r1] (raw file):
You can either pass them as one argument, semicolon-separated; or on separate arguments. The difference is that only the last statement in each argument has its results printed to stderr. I could not get the intuitive behavior (print all results, always) to work, because that would require a full parser on the client side for splitting the semicolons.

---

cli/cli_test.go, line 483 [r1] (raw file):
The format is intended for automated processing, you want minimum variability there. Any better suggestion?

---

Comments from the review on Reviewable.io

[petermattis]

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

cli/cli_test.go, line 475 [r1] (raw file):
Definitely don't want to split on semicolons on the client side. This is fine.

---

cli/cli_test.go, line 483 [r1] (raw file):
0 rows, 1 row, 2 rows. Automated processing will just grab the first field.

---

Comments from the review on Reviewable.io

[knz]

Review status: 1 of 5 files reviewed at latest revision, 3 unresolved discussions.

---

cli/flags.go, line 156 [r1] (raw file):
It is in the source file...

---

cli/flags.go, line 264 [r1] (raw file):
Done (in cli/context.go)

---

cli/sql.go, line 142 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[petermattis]

LGTM

---

Review status: 1 of 5 files reviewed at latest revision, 4 unresolved discussions.

---

cli/cli_test.go, line 483 [r2] (raw file):
Shouldn't this be 1 row now?

---

cli/context.go, line 2 [r2] (raw file):
This file needs the standard copyright notice.

---

cli/sql.go, line 147 [r2] (raw file):
Yowzer! Go doesn't make that easy.

---

cli/sql.go, line 169 [r2] (raw file):
s/single/Single/g

---

Comments from the review on Reviewable.io

[knz]

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

---

cli/cli_test.go, line 483 [r2] (raw file):
Done.

---

cli/context.go, line 2 [r2] (raw file):
Done.

---

cli/sql.go, line 169 [r2] (raw file):
Done.

---

Comments from the review on Reviewable.io

[petermattis]

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

---

cli/context.go, line 1 [r3] (raw file):
s/2015/2016/g. It isn't necessary to update old copyrights, but new ones should use the correct year.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 3 of 5 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

cli/cli_test.go, line 482 [r3] (raw file):
how does this work? no delimiter?

---

cli/cli_test.go, line 489 [r3] (raw file):
again, how does this work?

---

cli/context.go, line 42 [r3] (raw file):
does this need to be a separate function? if yes, does it need to be exported? in fact, seems like none of the identifiers here need to be exported.

---

cli/flags.go, line 261 [r3] (raw file):
in mysql, the value of this flag is the query string sent, not all subsequent arguments. any reason to deviate here?

---

cli/sql.go, line 147 [r2] (raw file):
Yowzer indeed. Can you add a comment explaining this pluralization magic?

---

cli/sql.go, line 43 [r3] (raw file):
how come?

---

cli/sql.go, line 79 [r3] (raw file):
instead of this, can you pass an error back up? you can use cobra's RunE field instead of Run for this purpose.

---

cli/sql.go, line 134 [r3] (raw file):
can you add a comment explaining why the newline is needed?

---

cli/sql.go, line 137 [r3] (raw file):
same comment about error handling.

---

cli/sql.go, line 150 [r3] (raw file):
this should use a tabwriter

---

cli/sql.go, line 159 [r3] (raw file):
either preserve tobi's TODO or return an error from here.

---

Comments from the review on Reviewable.io

[knz]

Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

cli/cli_test.go, line 482 [r3] (raw file):
look at the few lines above. The test has the different SQL statements in different command-line arguments. However at run-time the test framework does not print neither delimiter nor quotes for separate arguments.

---

cli/cli_test.go, line 489 [r3] (raw file):
see above in the code: c.RunWithArgs([]string{"sql","-e","begin","select 3","commit"})

---

cli/context.go, line 42 [r3] (raw file):
This is done like this for consistency with the other contexts.

---

cli/flags.go, line 261 [r3] (raw file):
Yes, see my previous answer to Peter in reply to the same question from him. Btw it also "Just Works (TM)" if you pass a single string after -e with statements separated by semicolons.

---

cli/sql.go, line 147 [r2] (raw file):
Oh come on. This is the only way to do conditional expressions in Go, I though this is common knowledge?

---

cli/sql.go, line 43 [r3] (raw file):
The command now properly exits with a status code indicating error when errors happen.

---

cli/sql.go, line 79 [r3] (raw file):
This would deserve another commit since it would touch all the commands. I can do this separately.

---

cli/sql.go, line 134 [r3] (raw file):
I could, if I knew why. The previous code was doing the same.

---

cli/sql.go, line 137 [r3] (raw file):
same answer.

---

cli/sql.go, line 150 [r3] (raw file):
What is a tabwriter?

---

cli/sql.go, line 159 [r3] (raw file):
The command properly terminates with an error code so the TODO can go. If you want to change the interface for the run functions for all commands, this should go to a different PR.

---

Comments from the review on Reviewable.io

[knz]

@tamird re the tabwriter question. A tabwriter does not make any sense: the purpose of this output format is to machine-parsable, and for this you want a single character as delimiter. It does not matter that it aligns well for the human observer.

There is a case to be made to allow multiple output formats in there, with or without proper escaping of strings, but I gather we can issue a separate PR to extend in this direction and the current form is probably OK for Beta.

[knz]

Review status: 4 of 5 files reviewed at latest revision, 12 unresolved discussions.

---

cli/context.go, line 1 [r3] (raw file):
Done.

---

Comments from the review on Reviewable.io

[knz]

Review status: 4 of 5 files reviewed at latest revision, 12 unresolved discussions.

---

cli/sql.go, line 79 [r3] (raw file):
I filed #3986 for this.

---

cli/sql.go, line 159 [r3] (raw file):
I filed #3986 for this.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 1 of 2 files at r4.
Review status: 4 of 5 files reviewed at latest revision, 5 unresolved discussions.

---

cli/context.go, line 42 [r3] (raw file):
Consistency is a bad reason to introduce dead API surface.

---

cli/flags.go, line 261 [r3] (raw file):
Sorry, where is the previous discussion? Can you link to it? Also, this does "just work" if you pass a string after -e, but it fails if you pass other flags after -e 'select 1'

---

cli/sql.go, line 147 [r2] (raw file):
Does this pattern appear anywhere else in the project?

---

cli/sql.go, line 134 [r3] (raw file):
Um, ok...does this work without it? I'm not OK with cargo-culting stuff around without even understanding why it's necessary.

---

Comments from the review on Reviewable.io