sql/parser: Support ANY/SOME/ALL array operators

[nvanbenschoten]

Closes #10519.

This change adds support for the ANY, SOME, and ALL array
operators, which are needed for Hibernate support. See
https://www.postgresql.org/docs/9.6/static/functions-comparisons.html
for details.

This is a WIP because I think some of the type checking code can be
cleaned up before merging.

---

This change is 

[jordanlewis]

Nice! I wonder if this could become cleaner by adding a new type for the quantifier operators. See comments below.

---

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

---

pkg/sql/parser/eval.go, line 1447 at r1 (raw file):

}

func evalArrayCmp(

I think a brief header comment on this function could be really helpful, e.g. evalArrayCmp evaluates the comparison in the scope of subOp (or quantifier could be a better name? see below) against the array right.

---

pkg/sql/parser/eval.go, line 1449 at r1 (raw file):

func evalArrayCmp(
	ctx *EvalContext,
	subOp ComparisonOperator,

Will this function horribly break if subOp isn't any/some/all? Should we check that condition? Alternatively, would it make any sense to break any/some/all out into their own new type? They aren't really ComparisonOperators after all - they're quantifiers.

---

pkg/sql/parser/eval.go, line 1452 at r1 (raw file):

	fn CmpOp,
	left, right Datum,
	all bool,

Perhaps this argument could be deleted? This function has all the information it needs to decide the value of all since it is passed the subOp.

---

pkg/sql/parser/eval.go, line 1457 at r1 (raw file):

	anyTrue := false
	sawNull := false
	for _, elem := range right.(*DArray).Array {

Does this cast need an error check?

---

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

type ComparisonExpr struct {
	Operator    ComparisonOperator
	SubOperator ComparisonOperator // used for array operators.

Hmm, so SubOperator is Any/Some/All? I don't think I would have any idea what this indicates if I was reading this code for the first time. Can you think of a better name? Perhaps Quantifier could be suitable, as in the existential/universal quantifiers in math which these operators correspond to. If you disagree, I think a less terse comment is warranted.

---

Comments from Reviewable

[nvanbenschoten]

Thanks for the review! I'm still working on cleaning some of this up, but as a general comment, Any, Some, and All are never the subOperators, they're always the Operators themselves. The subOperators (qualifiers) are the comparisons like = and < used by Any, Some, and All. This is important so that all specific handling of basic operators for things like normalization and index selection can stay the same. It also maintains consistency with In, which can be thought of as Any(=). In an ideal world, these operators could be defined like (Rust):

enum ComparisonOperator {
  EQ,
  LT,
  ...
  Any(ComparisonOperator),
}

Clearly, this needs improved documentation so that its not confusing.

---

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

---

Comments from Reviewable

[knz]

Reviewed 11 of 12 files at r1.
Review status: 11 of 12 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

pkg/sql/parser/eval.go, line 1449 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Will this function horribly break if subOp isn't any/some/all? Should we check that condition? Alternatively, would it make any sense to break any/some/all out into their own new type? They aren't really ComparisonOperators after all - they're quantifiers.

Agreed, these should have their own type. Granted they may not be enums but they cannot be anything else than any/some/all right?

---

pkg/sql/parser/sql.y, line 3639 at r1 (raw file):

    $$.val = &ComparisonExpr{Operator: NotIn, Left: $1.expr(), Right: $4.expr()}
  }
| a_expr subquery_op sub_type select_with_parens %prec CONCAT

Once #12111 is merged, I would favor a single rule here instead of two:

| a_expr subquery_op sub_type d_expr %prec CONCAT
{ ... }

This way we can then write 1 = ANY ARRAY[1,2,3], 1 = ANY current_schemas(true), etc without needing the parentheses (but the parentheses would still be allowed). What do you think?

---

pkg/sql/parser/type_check_test.go, line 124 at r1 (raw file):

		{`1 IN ('a', 'b')`, `unsupported comparison operator: 1 IN ('a', 'b'): expected 1 to be of type string, found type int`},
		{`1 IN (1, 'a')`, `unsupported comparison operator: 1 IN (1, 'a'): expected 1 to be of type string, found type int`},
		{`1 = ANY (2)`, `unsupported comparison operator: 1 = ANY 2: op ANY (array) requires array on right side`},

I'm not fond of this limitation, I think we should support ANY/SOME on tuples right away not just arrays.

---

Comments from Reviewable

[nvanbenschoten]

Thanks for the review @knz. Still cleaning some of this up but addresses a few questions,

---

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

---

pkg/sql/parser/eval.go, line 1449 at r1 (raw file):

Previously, knz (kena) wrote…

Agreed, these should have their own type. Granted they may not be enums but they cannot be anything else than any/some/all right?

@knz did you happen to see my comment above? any/some/all are not subOps, they're the primary operators.

---

pkg/sql/parser/sql.y, line 3639 at r1 (raw file):

Previously, knz (kena) wrote…

Once #12111 is merged, I would favor a single rule here instead of two:

| a_expr subquery_op sub_type d_expr %prec CONCAT
{ ... }

This way we can then write 1 = ANY ARRAY[1,2,3], 1 = ANY current_schemas(true), etc without needing the parentheses (but the parentheses would still be allowed). What do you think?

I'd be in favor of that because it would allow us to address the issue is brought up in normalize_test.go!

---

pkg/sql/parser/type_check_test.go, line 124 at r1 (raw file):

Previously, knz (kena) wrote…

I'm not fond of this limitation, I think we should support ANY/SOME on tuples right away not just arrays.

ANY/SOME is a strictly Postgres extension, and they only support arrays. Is it worth extending their extension even further?

---

Comments from Reviewable

[knz]

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

---

pkg/sql/parser/eval.go, line 1449 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

@knz did you happen to see my comment above? any/some/all are not subOps, they're the primary operators.

I think I did not understand it the first time round. Now it's clearer, thanks.

---

Comments from Reviewable

[cuongdo]

Any chance of getting this in early next week?

[nvanbenschoten]

After a bit of cleanup, a rebase, and some trial-and-error to convince myself that there isn't a nicer way to do the sub-operation stuff, this is ready for a full review now.

---

Review status: 1 of 12 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

pkg/sql/parser/eval.go, line 1447 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think a brief header comment on this function could be really helpful, e.g. evalArrayCmp evaluates the comparison in the scope of subOp (or quantifier could be a better name? see below) against the array right.

Done.

---

pkg/sql/parser/eval.go, line 1457 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Does this cast need an error check?

It doesn't because type checking and normalization will assure that only a DArray can reach here, but it makes more sense to make DArray part of the contract of the function and bring the assertion out to ComparisonExpr.Eval. Done.

---

pkg/sql/parser/sql.y, line 3639 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'd be in favor of that because it would allow us to address the issue is brought up in normalize_test.go!

Done. Changed a lot of tests for this too.

---

Comments from Reviewable

[knz]

Excellent, this looks much cleaner now. Just a nit about one of the main error messages.

---

Reviewed 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

pkg/sql/parser/sql.y, line 3717 at r2 (raw file):

    subOp, ok := $2.op().(ComparisonOperator)
    if !ok {
      sqllex.Error(fmt.Sprintf("op %s array requires operator to yield boolean", op))

I don't like this message because 1) it's not really informative and 2) it doesn't tell the user what to do to fix the problem. What about

Sprintf("%s %s <array> is invalid because "%s" is not a boolean operator", $2.op(), op, $2.op)

---

pkg/sql/parser/type_check_test.go, line 124 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

ANY/SOME is a strictly Postgres extension, and they only support arrays. Is it worth extending their extension even further?

I think they also work on subqueries. But I may be wrong.
Meanwhile this is really a side topic, so if that's really important we can revisit later. :)

---

Comments from Reviewable

[nvanbenschoten]

TFTR!

---

Review status: 9 of 12 files reviewed at latest revision, 6 unresolved discussions.

---

pkg/sql/parser/sql.y, line 3717 at r2 (raw file):

Previously, knz (kena) wrote…

I don't like this message because 1) it's not really informative and 2) it doesn't tell the user what to do to fix the problem. What about

Sprintf("%s %s <array> is invalid because "%s" is not a boolean operator", $2.op(), op, $2.op)

This was ripped directly from Postgres, but I like your phrasing better. Done.

---

pkg/sql/parser/type_check_test.go, line 124 at r1 (raw file):

Previously, knz (kena) wrote…

I think they also work on subqueries. But I may be wrong.
Meanwhile this is really a side topic, so if that's really important we can revisit later. :)

SGTM :)

---

Comments from Reviewable

[knz]

LGTM

I think you need to rebase as your test is hitting a bug in HandleSnapshot that was fixed last weekend.