sql: new built-in generator pg_get_keywords by knz · Pull Request #19127 · cockroachdb/cockroach

knz · 2017-10-09T14:25:44Z

Requested in #10291 (comment).

This PR includes some other useful refactors and cleanups.

cockroach-teamcity · 2017-10-09T14:25:55Z

This change is

justinj

LGTM (with two comments), I wonder why PG chose to make this a generator function rather than a system table.

justinj · 2017-10-09T14:37:33Z

pkg/sql/logictest/testdata/logic_test/generators

 -9223372036854775807
+
+# pg_get_keywords for compatibility (#10291)
+query TTT


Probably worth doing a colnames for this test?

justinj · 2017-10-09T14:57:18Z

pkg/sql/generator.go

-			}
-		}
+	columns := make(sqlbase.ResultColumns, len(tType.Cols))
+	for i := range columns {


I just realized PG lets you specify the label for a single-column generator:

justin=# select * from unnest(array[1,2,3]) a; a --- 1 2 3 (3 rows)

is that something we can easily support?

knz · 2017-10-09T15:20:43Z

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

pkg/sql/generator.go, line 62 at r4 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I just realized PG lets you specify the label for a single-column generator:
justin=# select * from unnest(array[1,2,3]) a;
 a
---
 1
 2
 3
(3 rows)
is that something we can easily support?

Awwww bad pg, bad. This is a weird mechanism. I'll investigate. However it's separate, it wasn't supported before this PR anyways.

pkg/sql/logictest/testdata/logic_test/generators, line 247 at r4 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Probably worth doing a colnames for this test?

Yes indeed. Done.

Comments from Reviewable

knz · 2017-10-09T15:49:56Z

Added a commit with the pg behavior, PTAL

nvb · 2017-10-09T18:15:31Z

Reviewed 2 of 2 files at r1, 4 of 4 files at r2, 6 of 6 files at r5, 4 of 4 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

pkg/sql/data_source.go, line 491 at r7 (raw file):

		// column name and table name.
		if vg, ok := src.plan.(*valueGenerator); ok &&
			(len(src.info.sourceAliases) == 0 ||

pull out these two conditions into variables

pkg/sql/parser/generator_builtins.go, line 237 at r2 (raw file):

// ResolvedType implements the ValueGenerator interface.
func (s *arrayValueGenerator) ResolvedType() TTable {
	return TTable{

Consider pulling this into a static variable too.

pkg/sql/parser/generator_builtins.go, line 220 at r5 (raw file):

// that pg_get_keywords returns deterministic results.
var keywordNames = func() []string {
	ret := make([]string, len(keywords))

nit: efficiency doesn't matter here so just set the capacity and append to get rid of i.

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

| /* EMPTY */
  {
    $$.val = Exprs{}

This is creating a zero-length Expr slice. I think you want a nil slice. Try Exprs(nil).

Comments from Reviewable

nvb · 2017-10-09T18:15:57Z

mostly just nits. Nice change!

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

Comments from Reviewable

…ccordingly

This patch makes the built-in definitions decide themselves the labels of the result columns. This in turn simplifies logical planning.

knz · 2017-10-09T20:11:50Z

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

pkg/sql/data_source.go, line 491 at r7 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

pull out these two conditions into variables

Done.

pkg/sql/parser/generator_builtins.go, line 237 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Consider pulling this into a static variable too.

Can't do, it's dependent on s.

pkg/sql/parser/generator_builtins.go, line 220 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: efficiency doesn't matter here so just set the capacity and append to get rid of i.

Done.

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is creating a zero-length Expr slice. I think you want a nil slice. Try Exprs(nil).

Done.

Comments from Reviewable

knz requested review from a team, justinj and nvb October 9, 2017 14:25

justinj approved these changes Oct 9, 2017

View reviewed changes

knz force-pushed the 20171009-get-keywords branch from 9549f26 to 897ba35 Compare October 9, 2017 15:19

knz force-pushed the 20171009-get-keywords branch 2 times, most recently from 803ec33 to 1d69e26 Compare October 9, 2017 16:03

knz added 2 commits October 9, 2017 22:05

sql/parser: introduce a new non-terminal opt_expr_list and simplify a…

fd76d29

…ccordingly

sql, sql/parser: simplify the handling of set-generator functions

f036880

This patch makes the built-in definitions decide themselves the labels of the result columns. This in turn simplifies logical planning.

knz force-pushed the 20171009-get-keywords branch from 1d69e26 to 4061df8 Compare October 9, 2017 20:13

knz added 3 commits October 9, 2017 23:07

sql/parser: new built-in pg_get_keywords for compatibility

4a31d26

sql/parser: lowercase keyword arguments upfront

52ecfdf

sql: emulate pg's behavior wrt AS clauses on set-generating functions

c9e887c

knz force-pushed the 20171009-get-keywords branch from 4061df8 to c9e887c Compare October 9, 2017 21:07

knz merged commit 4928740 into cockroachdb:master Oct 9, 2017

knz deleted the 20171009-get-keywords branch October 9, 2017 21:41

knz mentioned this pull request Oct 16, 2017

cherrypick-1.1: sql/parser: lowercase keyword arguments upfront #19280

Closed

jseldess mentioned this pull request Oct 18, 2017

sql: new built-in generator pg_get_keywords cockroachdb/docs#2073

Closed

jseldess mentioned this pull request Nov 16, 2017

Various PostgreSQL compatibility updates cockroachdb/docs#2175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: new built-in generator pg_get_keywords#19127

sql: new built-in generator pg_get_keywords#19127
knz merged 5 commits intocockroachdb:masterfrom
knz:20171009-get-keywords

knz commented Oct 9, 2017

Uh oh!

cockroach-teamcity commented Oct 9, 2017

Uh oh!

justinj left a comment

Uh oh!

justinj Oct 9, 2017

Uh oh!

justinj Oct 9, 2017 •

edited

Loading

Uh oh!

knz commented Oct 9, 2017

Uh oh!

knz commented Oct 9, 2017

Uh oh!

nvb commented Oct 9, 2017

Uh oh!

nvb commented Oct 9, 2017

Uh oh!

knz commented Oct 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

knz commented Oct 9, 2017

Uh oh!

cockroach-teamcity commented Oct 9, 2017

Uh oh!

justinj left a comment

Choose a reason for hiding this comment

Uh oh!

justinj Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

justinj Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knz commented Oct 9, 2017

Uh oh!

knz commented Oct 9, 2017

Uh oh!

nvb commented Oct 9, 2017

Uh oh!

nvb commented Oct 9, 2017

Uh oh!

knz commented Oct 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinj Oct 9, 2017 •

edited

Loading