sql/parser: better error messages for unimplemented features

[andreimatei]

Before this patch, error messages for unimplemented features detected by
the parser looked like:
reate type t as enum('a');
invalid syntax: statement ignored: at or near "a": syntax error: unimplemented: this syntax

after this patch, they spell out the missing feature:
invalid syntax: statement ignored: at or near "a": syntax error: unimplemented: enum types

The information about the feature is already present when creating the
error and was used as a "detail" for telemetry. Now I've made it part of
the error message too.

Release note (sql change): Error messages for unimplemented features
have been improved to spell out what the unimplemented feature is.

[cockroach-teamcity]

This change is 

[andreimatei]

I'm not sure this is the right way to go because I don't really understand what that "detail" that I've reused was. PTAL.

[awoods187]

This is cool! I love improving the error messages here.

[knz]

LGTM on the approach but there are two things that you need to take into account:

1. you'll need to fix the newly failing test. I think the way to do this is not just by changing the expected string, and instead by changing the provided detail. The text "unimplemented feature: *IndirectionExpr" is obscure and useless, it would be better for the user to see "unimplemented feature: computed indexes" (sql.go line 5216, linking to issue #9682).

2. while fixing point 1, be careful about not changing the detail argument to the unimplemented error, as this is the exact string that goes to telemetry. In telemetry (as opposed to the user's eyes) we do want to see *IndirectionExpr for the error linked to issue #9682. This is because a user attempting to use computed indexes may do so using various scalar expressions, and we want to find out int telemetry what are the expression types that are most commonly used.

In order to improve the user-facing detail while keeping the telemetry detail, you may need to introduce a variant of the unimplementedWithIssueDetail function that takes an extra argument.

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

[bobvawter]

All committers have signed the CLA.

[andreimatei]

You really sure we want BinaryExpr and IndirectionExpr in telemetry for the computed indexes? It gets to be a bit unwieldy to add support for a telemetry key different from the error message. I can't imagine much utility for it either; the type of the node at the top of the expression AST hardly characterizes the expression, and I imagine that if we add support for computed indexes, we'll support all sorts of expressions from the get go anyway.

Reviewable status: complete! 0 of 0 LGTMs obtained

[andreimatei]

I've done the thing to keep the message different from the telemetry key for the computed indexes, but I'd personally rather get rid of it.
I've also changed purposelyUnimplemented() to pass the details as the message.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 54558dcb.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[andreimatei]

friendly ping for when you're back Rafa

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

[knz]

Reviewed 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

[andreimatei]

Rafa please help me out. The linter says

    TestLint/TestRoachVet: lint_test.go:85:
        pkg/sql/parser/lexer.go:153:25: Unimplemented(): message argument is not a constant expression
        Tip: use YourFuncf("descriptive prefix %s", ...) or list new formatting wrappers in pkg/testutils/lint/passes/fmtsafe/functions.go.
        pkg/sql/parser/lexer.go:165:40: UnimplementedWithIssueDetail(): message argument is not a constant expression
        Tip: use YourFuncf("descriptive prefix %s", ...) or list new formatting wrappers in pkg/testutils/lint/passes/fmtsafe/functions.go.

This is about the lexer.Unimplemented() and lexer.UnimplementedWithIssueDetail() functions. I've tried a couple of things but I don't know how to shut it up. I can't use /* nolint:fmtsafe */ cause it's not test code. I tried adding these functions to the requireConstMsg list, but that doesn't seem to do the trick. The only thing that does the trick seems to be:

// UnimplementedWithIssueDetail wraps Error, setting lastUnimplementedError.
func (l *lexer) UnimplementedWithIssueDetail(issue int, detail string) {
	l.lastError = unimp.NewWithIssueDetailf(issue, detail /* detail */, "%s", detail)
	l.populateErrorDetails()
}

Is that what I was supposed to do?

Would it be reasonable to say that the body of the functions listed in requireConstMsg shouldn't be checked?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)