sql: new heuristic-based completion engine

[knz]

This PR extends the server-side completion logic available under SHOW COMPLETIONS.

Epic: CRDB-22182
Fixes #37355.
Intended for use together with #86457.

The statement now returns 5 columns: completion, category, description, start, end.
This change is backward-compatible with the CLI code in previous versions, which was not checking the number of columns.

It works roughly as follows:

1. first the input is scanned into a token array.

2. then the token array is translated to a sketch: a string with the
   same length as the token array, where each character corresponds to an
   input token and an extra character indicating where the completion was
   requested.

3. then the completion rules are applied in sequence. Each rule
   inspects the sketch (and/or the token sequence) and decides whether
   to do nothing (skip to next rule), or to run some SQL which will
   return a batch of completion candidates.

   Each method operates exclusively on the token sequence, and does
   not require the overall SQL syntax to be valid or complete.

Also, the completion engine executes in a streaming fashion, so that
there is no need to accumulate all the completion candidates in RAM
before returning them to the client. This prevents excessive
memory usage server-side, and offers the client an option to cancel
the search once enough suggestions have been received.

Code organization:

• new package sql/compengine: the core completion engine, also
  where sketches are computed.

  Suggested reading: api.go in the new package.

• new package sql/comprules: the actual completion
  methods/heuristics, where sketches are mapped into SQL queries
  that provide the completion candidates.

  Suggested reading: the test cases under testdata.

Some more words about sketches:

For example, SHOW COMPLETIONS AT OFFSET 10 FOR 'select myc from sc.mytable;' produces the sketch "ii'ii.i;" where i indicates an
identifier-like token and ' indicates the cursor position.

The purpose of the sketch is to simplify the pattern matching
performed by the completion heuristics, and enables the application of
regular expressions on the token sequence.

For example, a heuristic to complete schema-qualified relations in
the current database, or a database-qualified relation, would use
the regexp [^.;]i\.(['_]|i'): an identifier not following a
period or a semicolon, followed by a period, followed by the
completion cursor (with the cursor either directly after the period
or on an identifier after the period).

Supersedes #45186.

[cockroach-teamcity]

This change is 

[knz]

@rickystewart when you get a chance (NOT URGENT) could you help with this?

I've defined a new generator rule in pkg/sql/scanner that generates token_names_test.go.

Because it's generated, I'm not adding it to the srcs in the go_test() invocation.

However every time I run dev generate bazel, bazel/gazelle auto-adds it to the srcs list. Then if I run generate go the build starts failing because the file exists both in my work tree and in the sandbox.

Do you know how I can prevent dev generate bazel from adding the generated file to srcs?

[rickystewart]

Do you know how I can prevent dev generate bazel from adding the generated file to srcs?

Yes -- add a comment to BUILD.bazel (probably the one in your package subdirectory) like # gazelle:exclude token_names_test.go.

[knz]

thank you!

[knz]

@rickystewart hi, for when you have time (NOT URGENT), this PR could use some help:

There is a new generated file in sql/scanner (token_names_test.go).
The linter is now complaining that it cannot build/lint the code because this generated file is missing.

How can I add the generation of this file to the dependencies for the linter?

[knz]

Oh nvm, found it -- it's because I forgot to update the Makefile.

[knz]

This is now rebased, with more tests added, ready for review.

[rytaft]

Very cool! I just looked at the second commit (the one that updates the optimizer)

Reviewed 27 of 27 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @ZhouXing19)

---

pkg/sql/opt/exec/factory.opt line 730 at r2 (raw file):

define ShowCompletions {
    Command *tree.ShowCompletions
}

I think this needs to go to the end of this file to avoid causing problems with gists (see the comment on line 39 of plan_gist_factory.go)

---

pkg/sql/opt/exec/execbuilder/statement.go line 321 at r2 (raw file):

		return execPlan{}, err
	}
	// ControlSchedules returns no columns.

nit: update comment

---

pkg/sql/opt/exec/explain/testdata/gists line 1012 at r2 (raw file):

# cancelQueriesOp
gist-explain-roundtrip

Can you please add a test in this file for SHOW COMPLETIONS and make sure that none of these other tests change? Thank you!

[ZhouXing19]

This is so COOL! Thanks for adding such comprehensive tests as well.
Most of my comments are just for nits.

Reviewed 8 of 8 files at r1, 27 of 27 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, 23 of 23 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

---

pkg/sql/conn_executor_exec.go line 2007 at r5 (raw file):

) error {
	res.SetColumns(ctx, colinfo.ShowCompletionsColumns)
	log.Warningf(ctx, "COMPLETION GENERATOR FOR: %+v", *n)

I'm a bit concerned about the usage of the internal executor here (see #91004 if you're curious). Could you add a comment:

TODO(janexing): better bind the internal executor with the txn.

Thanks!

---

pkg/sql/compengine/sketch.go line 54 at r5 (raw file):

	var buf strings.Builder
	if offset < int(tokens[0].Start) {
		buf.WriteByte('_')

nit: change it to buf.WriteByte(byte(MarkCursorInSpace))

---

pkg/sql/compengine/sketch.go line 76 at r5 (raw file):

	// Requested offset at or beyond EOF.
	if cursorPosition == 0 && offset >= int(tokens[len(tokens)-1].Start) {

nit: iiuc, the last token is always the EOF, so the start and the end indexes are the same. It may be easier to understand if we have >= nt(tokens[len(tokens)-1].End).

---

pkg/sql/compengine/sketch.go line 82 at r5 (raw file):

		// outside of the string, so that indexing using them is always
		// possible.
		offset = int(tokens[len(tokens)-1].Start)

ditto

---

pkg/sql/compengine/sketch.go line 144 at r5 (raw file):

			pos, len(sketch), sketch)
	}
	if sketch[pos] != '_' && sketch[pos] != '\'' {

nit: change it to if sketch[pos] != MarkCursorInSpace && sketch[pos] != MarkCursorInToken

---

pkg/sql/comprules/rules.go line 156 at r5 (raw file):

	c.Trace("completing for %q (%d,%d)", prefix, start, end)
	// TODO(knz): use the comment system instead of crdb_internal.

Just out of curiosity -- could you say more about the "comment system"?

---

pkg/sql/comprules/rules.go line 174 at r5 (raw file):

// A database name can only occur after a keyword or a comma (,).
var compDbRe = regexp.MustCompile(`i(i'|_)|,(['_]|i')`)

nit: For this regex, seems that sketches ,', ,_ and ,i also match. Is this expected? Should we ask for an identifier token at the beginning?

---

pkg/sql/compengine/sketch_test.go line 54 at r5 (raw file):

			fmt.Fprintln(tw, "--")
			c := completions{tokens: tokens, sketch: sketch, queryTokIdx: pos}
			for i := -2; i <= 2; i++ {

nit: could you add a comment for this? IIUC we're printing out the token corresponding to the current cursor, and also its surrounding tokens.

---

pkg/sql/completions.go line 41 at r5 (raw file):

	txn := params.p.Txn()
	queryIterFn := func(ctx context.Context, opName string, stmt string, args ...interface{}) (sqlutil.InternalRows, error) {
		return ie.QueryIteratorEx(ctx, opName, txn,

We can just use params.p.QueryIteratorEx() here.
initInternalExecutor() is expected to be used only in planner.WithInternalExecutor() or query/exec functions such as planner.ExecEx().

---

pkg/sql/completions_test.go line 1 at r3 (raw file):

// Copyright 2022 The Cockroach Authors.

nit: We can remove this file.

[knz]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft and @ZhouXing19)

---

pkg/sql/conn_executor_exec.go line 2007 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

I'm a bit concerned about the usage of the internal executor here (see #91004 if you're curious). Could you add a comment:

TODO(janexing): better bind the internal executor with the txn.

Thanks!

Done.

---

pkg/sql/compengine/sketch.go line 54 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: change it to buf.WriteByte(byte(MarkCursorInSpace))

Done.

---

pkg/sql/compengine/sketch.go line 76 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: iiuc, the last token is always the EOF, so the start and the end indexes are the same. It may be easier to understand if we have >= nt(tokens[len(tokens)-1].End).

Done.

---

pkg/sql/compengine/sketch.go line 82 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

ditto

Done.

---

pkg/sql/compengine/sketch.go line 144 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: change it to if sketch[pos] != MarkCursorInSpace && sketch[pos] != MarkCursorInToken

Done.

---

pkg/sql/comprules/rules.go line 156 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Just out of curiosity -- could you say more about the "comment system"?

Things from pg_catalog. Explained in comment.

---

pkg/sql/comprules/rules.go line 174 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: For this regex, seems that sketches ,', ,_ and ,i also match. Is this expected? Should we ask for an identifier token at the beginning?

matching on ,' doesn't seem right, good catch. Modified.

,_ is expected, because we have select * from db1.t1, db2.t2, we want to see databases listed after the comma.

---

pkg/sql/opt/exec/factory.opt line 730 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I think this needs to go to the end of this file to avoid causing problems with gists (see the comment on line 39 of plan_gist_factory.go)

Done.

---

pkg/sql/opt/exec/execbuilder/statement.go line 321 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: update comment

Good catch.

---

pkg/sql/opt/exec/explain/testdata/gists line 1012 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Can you please add a test in this file for SHOW COMPLETIONS and make sure that none of these other tests change? Thank you!

Good idea. Done.

---

pkg/sql/compengine/sketch_test.go line 54 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: could you add a comment for this? IIUC we're printing out the token corresponding to the current cursor, and also its surrounding tokens.

Done.

---

pkg/sql/completions.go line 41 at r5 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

We can just use params.p.QueryIteratorEx() here.
initInternalExecutor() is expected to be used only in planner.WithInternalExecutor() or query/exec functions such as planner.ExecEx().

Done.

[knz]

TFYRs!

bors r=rytaft,ZhouXing19

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)