sql/parser: Introduce IndirectionExpr and support for array subscripting #10669

nvanbenschoten · 2016-11-14T05:43:06Z

This change adds support for subscripting ARRAY expressions by introducing an IndirectionExpr node. This is the same approach that Postgres uses to represent indirection in its AST.

This change is

knz · 2016-11-14T11:49:18Z

Reviewed 4 of 4 files at r1, 10 of 10 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

pkg/sql/pgwire_test.go, line 281 at r2 (raw file):

      "SELECT ($1 + $1) + CURRENT_DATE()":         "pq: could not determine data type of placeholder $1",
      "SELECT $1 + $2, $2::FLOAT":                 "pq: could not determine data type of placeholder $1",
      "SELECT $1[2]":                              "pq: could not determine data type of placeholder $1",

Please add a test that demonstrates when the placeholder at this syntax position can be properly typed.

pkg/sql/parser/eval.go, line 1908 at r2 (raw file):

          }
          if i > 0 {
              return nil, util.UnimplementedWithIssueErrorf(2115, "multidimensional ARRAY %s", expr)

Even though the rest of the code is not ready I think you can remove this condition here -- the Eval code is pretty much OK as it is for when the rest of the machinery supports multi-dimensional arrays.

(There is perhaps a case to be made that we could compute a global index from a multi-dimensional index, assuming the array is homogeneous, but if that's what we want to do then the condition must be before the loop -- if len(expr.Indirection) > 0 ...)

pkg/sql/parser/eval.go, line 1920 at r2 (raw file):

          subscriptIdx = int(*d.(*DInt))
      default:
          return nil, errors.Errorf("unhandled indirection type %T", t)

Perhaps more informative "syntax not yet supported: %s", expr.Indirection?

pkg/sql/parser/parse_test.go, line 304 at r2 (raw file):

      {`SELECT ((SELECT 1))`},
      {`SELECT (SELECT ARRAY['a', 'b'])[2]`},
      {`SELECT ((SELECT ARRAY['a', 'b']))[2]`},

Why not also SELECT (ARRAY['a', 'b'])[2] ?

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

          }
          if i > 0 {
              return nil, util.UnimplementedWithIssueErrorf(2115, "multidimensional ARRAY %s", expr)

See above - I think you're doing fine without this check.
define desiredType := desired before the beginning of the loop;
then desiredType = tArray{desiredType} inside the loop,
then use that below.

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

          t.Begin = beginExpr
      default:
          return nil, errors.Errorf("unhandled indirection type %T", t)

See above - "syntax not yet supported"

Comments from Reviewable

nvanbenschoten · 2016-11-16T01:03:21Z

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

pkg/sql/pgwire_test.go, line 281 at r2 (raw file):