sql: Add the builtin function _pg_expandarray()#24422
sql: Add the builtin function _pg_expandarray()#24422craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
| makeGeneratorBuiltinWithReturnType( | ||
| tree.ArgTypes{{"input", types.AnyArray}}, | ||
| func(args []tree.TypedExpr) types.T { | ||
| if len(args) == 0 { |
There was a problem hiding this comment.
len(args) == 0 is impossible since the function always accepts exactly one argument.
There was a problem hiding this comment.
Actually, it is needed or else this testcase will panic:
query error pq: information_schema\._pg_expandarray\(\): cannot determine type of empty array\. Consider annotating with the desired type, for example ARRAY\[\]:::int\[\]
SELECT information_schema._pg_expandarray(ARRAY[])There was a problem hiding this comment.
-
That's a different case, you would test that with
len(args[0]) == 0, notlen(args) == 0. (but please don't do that) -
that error is not a panic, it's a legitimate error and it is expected.
array[]without an explicit type will give that error in any context, no tjust with your new built-in. Make the test case usearray[]:::int[]and see what happens.
There was a problem hiding this comment.
I already added the test cases for array[]:::int[].
So, to be clear, by removing the if len(args) == 0.
SELECT information_schema._pg_expandarray(ARRAY[])Results in a panic and not an error. It's that if condition that returns the tree.UnknownReturnType error:
panic: runtime error: index out of range
goroutine 1 [running]:
github.com/cockroachdb/cockroach/pkg/sql/sem/builtins.glob..func191(0x0, 0x0, 0x0, 0x527eee0, 0xc420502300)
/Users/bram/go/src/github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/generator_builtins.go:107 +0x1dc
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.returnTypeToFixedType(0x4aee5e0, 0xc4201b3a20, 0x13)
/Users/bram/go/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/overload.go:265 +0x3b
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.Builtin.FixedReturnType(0x4be1540, 0xc420502320, 0x4aee5e0, 0x100, 0x3, 0x4a673dd, 0xd, 0x4a8dcc5, 0x36, 0x0, ...)
/Users/bram/go/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/builtin.go:125 +0x30
main.generateFunctions(0xc42012c000, 0x1, 0x2, 0xc4205001e0, 0x1f)
/Users/bram/go/src/github.com/cockroachdb/cockroach/pkg/cmd/docgen/funcs.go:192 +0x37e
main.init.0.func1(0xc4200ce240, 0xc420465960, 0x1, 0x2, 0x0, 0x0)
/Users/bram/go/src/github.com/cockroachdb/cockroach/pkg/cmd/docgen/funcs.go:52 +0x171
github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra.(*Command).execute(0xc4200ce240, 0xc420465940, 0x2, 0x2, 0xc4200ce240, 0xc420465940)
/Users/bram/go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:698 +0x46d
github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc4200ce900, 0x4a75f49, 0x1e, 0xc4200ce900)
/Users/bram/go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:783 +0x2e4
github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra.(*Command).Execute(0xc4200ce900, 0x1, 0xc4200ce240)
/Users/bram/go/src/github.com/cockroachdb/cockroach/vendor/github.com/spf13/cobra/command.go:736 +0x2b
main.main()
/Users/bram/go/src/github.com/cockroachdb/cockroach/pkg/cmd/docgen/main.go:37 +0x27
There was a problem hiding this comment.
@nvanbenschoten can you advise what is the best course of action here.
|
Very nice. I am flabbergasted on how elegant/simple this is. |
|
Just added a few extra test cases for edge cases. |
|
s/froming/forming/ in commit description Reviewed 1 of 3 files at r1, 1 of 2 files at r2. pkg/sql/sem/builtins/generator_builtins.go, line 107 at r1 (raw file): Previously, knz (kena) wrote…
This isn't great, but I think it's correct at the moment (see Comments from Reviewable |
This adds the information_schema._pg_expandarray() function. It is needed for compatibility with a number of ORMs. See cockroachdb#16971. information_schema._pg_expandarray takes an array and returns a set of the value and an index into the array. This is a very old function and from what I can discern, was designed for internal use only, but was picked up by a few ORMs. There is no real supporting documentation for the feature. The code for it can be seen here: https://sourcegraph.com/github.com/postgres/postgres/-/blob/src/backend/catalog/information_schema.sql#L43:17 Furthermore, if the function is a Set Retruning Function, it returns tuples when evaluated in a scalar context: In Postgres: ```sql SELECT information_schema._pg_expandarray(array['i','i','o','t','b']); _pg_expandarray ----------------- (i,1) (i,2) (o,3) (t,4) (b,5) (5 rows) ``` With this patch Cockroach now supports this directly as well: ```sql SELECT information_schema._pg_expandarray(array['i','i','o','t','b']); +------------------------------------+ | information_schema._pg_expandarray | +------------------------------------+ | ('i',1) | | ('i',2) | | ('o',3) | | ('t',4) | | ('b',5) | +------------------------------------+ (5 rows) ``` However, when evaluating an SRF in the `FROM` context, it should return columns. Luckily, we already do exactly that. This is true for `unnest()` as well as the new `_pg_expandarray`. The difference is a little subtle and I couldn't find any good documentation on it, but this answer seems to cover it quite well: https://dba.stackexchange.com/questions/172463/understanding-set-returning-function-srf-in-the-select-list In Postgres: ```sql SELECT * FROM information_schema._pg_expandarray(array['i','i','o','t','b']); x | n ---+--- i | 1 i | 2 o | 3 t | 4 b | 5 (5 rows) ``` In Cockroach: ```sql SELECT * FROM information_schema._pg_expandarray(array['i','i','o','t','b']); +---+---+ | x | n | +---+---+ | i | 1 | | i | 2 | | o | 3 | | t | 4 | | b | 5 | +---+---+ (5 rows) ``` Note that this function with the FROM context is essentially the same as: ```sql SELECT * FROM unnest(array['i','1','3','r']) with ordinality as c(x,n); +---+---+ | x | n | +---+---+ | i | 1 | | 1 | 2 | | 3 | 3 | | r | 4 | +---+---+ (4 rows) ``` But to retain direct compatibility with Postgres, the original SET response needs to be maintained as well. Part of cockroachdb#16971. Release note (sql change): Added support for the information_schema._pg_expandarray() function.
|
Thanks. I guess it should be FROMing. Just re-worded to make it clearer. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion. Comments from Reviewable |
|
@knz, just waiting on a final LGTM and I'll get bors to merge this. |
|
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. Comments from Reviewable |
|
bors r=knz |
24422: sql: Add the builtin function _pg_expandarray() r=knz a=BramGruneir This adds the information_schema._pg_expandarray() function. It is needed for compatibility with a number of ORMs. See #16971. information_schema._pg_expandarray takes an array and returns a set of the value and an index into the array. This is a very old function and from what I can discern, was designed for internal use only, but was picked up by a few ORMs. There is no real supporting documentation for the feature. The code for it can be seen here: https://sourcegraph.com/github.com/postgres/postgres/-/blob/src/backend/catalog/information_schema.sql#L43:17 Furthermore, if the function is a Set Retruning Function, it returns tuples when evaluated in a scalar context: In Postgres: ```sql SELECT information_schema._pg_expandarray(array['i','i','o','t','b']); _pg_expandarray ----------------- (i,1) (i,2) (o,3) (t,4) (b,5) (5 rows) ``` With this patch Cockroach now supports this directly as well: ```sql SELECT information_schema._pg_expandarray(array['i','i','o','t','b']); +------------------------------------+ | information_schema._pg_expandarray | +------------------------------------+ | ('i',1) | | ('i',2) | | ('o',3) | | ('t',4) | | ('b',5) | +------------------------------------+ (5 rows) ``` However, when evaluating an SRF in the `FROM` context, it should return columns. Luckily, we already do exactly that. This is true for `unnest()` as well as the new `_pg_expandarray`. The difference is a little subtle and I couldn't find any good documentation on it, but this answer seems to cover it quite well: https://dba.stackexchange.com/questions/172463/understanding-set-returning-function-srf-in-the-select-list In Postgres: ```sql SELECT * FROM information_schema._pg_expandarray(array['i','i','o','t','b']); x | n ---+--- i | 1 i | 2 o | 3 t | 4 b | 5 (5 rows) ``` In Cockroach: ```sql SELECT * from information_schema._pg_expandarray(array['i','i','o','t','b']); +---+---+ | x | n | +---+---+ | i | 1 | | i | 2 | | o | 3 | | t | 4 | | b | 5 | +---+---+ (5 rows) ``` Note that after froming the resulting set to a table, this function is essentially the same as: ```sql SELECT * from unnest(array['i','1','3','r']) with ordinality as c(x,n); +---+---+ | x | n | +---+---+ | i | 1 | | 1 | 2 | | 3 | 3 | | r | 4 | +---+---+ (4 rows) ``` But to retain direct compatibility with Postgres, the original SET response needs to be maintained as well. Part of #16971. Release note (sql change): Added support for the information_schema._pg_expandarray() function.
Build succeeded |
This adds the information_schema._pg_expandarray() function. It is needed for
compatibility with a number of ORMs. See #16971.
information_schema._pg_expandarray takes an array and returns a set of the
value and an index into the array. This is a very old function and from what I
can discern, was designed for internal use only, but was picked up by a few
ORMs. There is no real supporting documentation for the feature. The code for
it can be seen here:
https://sourcegraph.com/github.com/postgres/postgres/-/blob/src/backend/catalog/information_schema.sql#L43:17
Furthermore, if the function is a Set Retruning Function, it returns tuples
when evaluated in a scalar context:
In Postgres:
With this patch Cockroach now supports this directly as well:
However, when evaluating an SRF in the
FROMcontext, it should returncolumns. Luckily, we already do exactly that. This is true for
unnest()aswell as the new
_pg_expandarray. The difference is a little subtle and Icouldn't find any good documentation on it, but this answer seems to cover it
quite well:
https://dba.stackexchange.com/questions/172463/understanding-set-returning-function-srf-in-the-select-list
In Postgres:
In Cockroach:
Note that after froming the resulting set to a table, this function is
essentially the same as:
But to retain direct compatibility with Postgres, the original SET response
needs to be maintained as well.
Part of #16971.
Release note (sql change): Added support for the
information_schema._pg_expandarray() function.