sql: properly handle parenthesized/nested tuples as operands for ANY/ALL operations #18266

richardwu · 2017-09-06T20:31:41Z

Type checking for tuples in ANY/ALL operations were added in #18094.

It was initially implemented such that it assumed the right Expr was either a literal tuple (of which it would require every element be equivalent to the type of left after upcasting), an array, or a subquery.

We did not however check for tuples nested within parentheses, thus a semantically-incorrect expression such as

SELECT 1 = ANY (((1.0, 1.1)))

would not type error but succeed.

cockroach-teamcity · 2017-09-06T20:31:47Z

This change is

justinj · 2017-09-06T22:03:01Z

Can you see if there's a way this can be done without AnnotateParens like how we are able to typecheck something like (1.2) + ((1)) without it? It seems fishy to me that we need this extra mechanism now but we didn't before. If not could you add a (brief) comment explaining why to AnnotateParens?

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

pkg/sql/parser/expr.go, line 272 at r1 (raw file):

	exprParen := expr
	for {
		if p, ok := exprParen.(*ParenExpr); ok {

I think you can maybe write this with fewer jumpy statements:

for {
  if ... ; ok {
    p.typ = typ
    exprParen = p.Expr
  } else {
    return expr
  }
}

?

Also, you never use the return value of this so consider either making this not return a value or using its return value. Since it's mutative I would lean towards not returning a value to emphasize that.

pkg/sql/parser/expr_test.go, line 237 at r1 (raw file):

	}

	for i, test := range testExprs {

you can make these subtests (which can give you slightly cleaner output) if you use t.Run...

pkg/sql/parser/expr_test.go, line 244 at r1 (raw file):

		annotated := AnnotateParens(expr, typ)

		n := 0

I think you can put this on the for line

pkg/sql/parser/type_check.go, line 913 at r1 (raw file):

// For example, (1, 2.5) with required TypeDecimal would raise a sane error whereas (1.0, 2.5) with required TypeDecimal would pass.
func typeCheckAndRequireTupleElems(ctx *SemaContext, expr Expr, required Type) (TypedExpr, error) {
	// Retrieve reference to (possibly nested) tuple expression

nit: period

pkg/sql/parser/type_check.go, line 925 at r1 (raw file):

		tuple.types[i] = typedExpr.ResolvedType()
	}
	// We cannot simply return tuple since we want the original expression returned and with

super nit: wrap "tuple" with backticks so it's clearer that this is referring to a variable

Comments from Reviewable

richardwu · 2017-09-06T23:13:08Z

Thanks for bringing this up! I decided to leave in AnnotateParens since another part of the code seems to use it; my guess is that codepath could also be refactored to do the same.

I've verified that the types are propagated to the surrounding ParenExprs using log statements. I don't think there's a clean/kosher way to test that the type annotations propagate, thoughts?

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

pkg/sql/parser/expr.go, line 272 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I think you can maybe write this with fewer jumpy statements:
for {
  if ... ; ok {
    p.typ = typ
    exprParen = p.Expr
  } else {
    return expr
  }
}
?

Also, you never use the return value of this so consider either making this not return a value or using its return value. Since it's mutative I would lean towards not returning a value to emphasize that.

Done.

pkg/sql/parser/expr_test.go, line 237 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

you can make these subtests (which can give you slightly cleaner output) if you use t.Run...

Done.

pkg/sql/parser/expr_test.go, line 244 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I think you can put this on the for line

I needed to scope the n so I can compare it with test.count afterwards.

pkg/sql/parser/type_check.go, line 913 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: period

Removed comment so n/a anymore.

pkg/sql/parser/type_check.go, line 925 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

super nit: wrap "tuple" with backticks so it's clearer that this is referring to a variable

Removed comment so n/a anymore.

Comments from Reviewable

justinj · 2017-09-06T23:20:41Z

(I think you didn't push up your changes)

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

Comments from Reviewable

justinj · 2017-09-06T23:31:34Z

One more thing I don't fully understand - why do we need to type every layer of parentheses rather than just typing the outermost ones?

Otherwise LGTM but @knz might have comments so wait for him to take a look.

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

pkg/sql/parser/expr.go, line 278 at r2 (raw file):

			return
		}

nit: extra line

pkg/sql/parser/expr_test.go, line 244 at r1 (raw file):

Previously, richardwu (Richard Wu) wrote…

I needed to scope the n so I can compare it with test.count afterwards.

Ah yeah, my bad!

Comments from Reviewable

knz · 2017-09-07T12:26:18Z

I think you're still missing one intellectual step about the power of functional rewrites.
The entire TypeCheck function is free to replace arbitrary sub-trees of an expression by another.

In particular, it is valid to remove any remaining ParenExpr here. So strip away! And don't bother to "annotate types on them". See my comments below for some suggestions, but please also grep for ParenExpr throughout the parser and sql packages, and replace all such loops by StripParens. There is no honor in respecting parentheses. They are a purely grammatical construct, they have no semantic value whatsoever and do not deserve to be preserved.

Reviewed 1 of 4 files at r1, 2 of 3 files at r2.
Review status: 3 of 4 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

pkg/sql/parser/expr.go, line 269 at r2 (raw file):

// AnnotateParens annotates the parentheses surrounding the first non-parentheses
// expression with the given type.
func AnnotateParens(expr Expr, typ Type) {

wut? no!!

😆

pkg/sql/parser/type_check.go, line 918 at r2 (raw file):

	// If the expression is wrapped in parentheses, do a recursive call
	// and type annotate the parentheses with the resulting tuple type.
	if parenExpr, ok := expr.(*ParenExpr); ok {

Man you're making this so complicated!

Replace this entire conditional by

expr = StripParens(expr)

bam!

pkg/sql/parser/type_check.go, line 980 at r2 (raw file):

	var cmpTypeLeft Type
	var cmpTypesRight []Type
	var leftTyped, rightTyped TypedExpr

Insert left = StripParens(left); right = StripParens(right) here.

pkg/sql/parser/type_check.go, line 981 at r2 (raw file):

	var cmpTypesRight []Type
	var leftTyped, rightTyped TypedExpr
	if array, isConstructor := StripParens(right).(*Array); isConstructor {

Remove the call to StripParens here.

pkg/sql/parser/type_check.go, line 1006 at r2 (raw file):

		array.typ = TArray{retType}

		AnnotateParens(right, array.typ)

Remove (see above -- the parentheses are gone if you arrive here.)

pkg/sql/parser/type_check.go, line 1024 at r2 (raw file):

		cmpTypeLeft = leftTyped.ResolvedType()

		if _, ok := StripParens(right).(*Tuple); ok {

remove the call to StripParens here, already done above

Comments from Reviewable

richardwu · 2017-09-07T14:10:23Z

I'm wondering if we might run into bugs later on with precedence when we serialize it for distsql. For example, this came up with negative numbers #15617.

I can't think of an example off the top of my head (perhaps someone more familiar with SQL grammar gotchas would fare better) where serializing the functional rewrite of an expression without parentheses would cause mis-parsing, but I think you're absolutely correct!

Comments from Reviewable

richardwu · 2017-09-07T15:29:10Z

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions.

pkg/sql/parser/type_check.go, line 918 at r2 (raw file):