Review: UNNEST (and other functions) WITH ORDINALITY
On 17 June 2013 06:33, David Fetter <david@fetter.org> wrote:
Next revision of the patch, now with more stability. Thanks, Andrew!
Rebased vs. git master.
Here's my review of the WITH ORDINALITY patch.
Overall I think that the patch is in good shape, and I think that this
will be a very useful new feature, so I'm keen to see this committed.
All the basic stuff is OK --- the patch applies cleanly, compiles with
no warnings, and has appropriate docs and new regression tests which
pass. I also tested it fairly thoroughly myself, and I couldn't break
it. Everything worked as I expected, and it works nicely with LATERAL.
I have a few minor comments that should be considered before passing
it on to a committer:
1). In func.sgml, the new doc example "unnest WITH ORDINALITY" is
mislablled, since it it's not actually an example of unnest(). Also it
doesn't really belong in that code example block, which is about
generate_subscripts(). I think that there should probably be a new
sub-section starting at that point for WITH ORDINALITY. Perhaps it
should start with a brief paragraph explaining how WITH ORDINALITY can
be applied to functions in the FROM clause of a query.
[Actually it appears that WITH ORDINALITY works with non-SRF's too,
but that's less useful, so I think that the SRF section is probably
still the right place to document this]
It might also be worth mentioning here that currently WITH ORDINALITY
is not supported for functions that return records.
In the code example itself, the prompt should be trimmed down to match
the previous examples.
2). In the SELECT docs, where function_name is documented, I would be
tempted to start a new paragraph for the sentence starting "If the
function has been defined as returning the record data type...", since
that's really a separate syntax. I think that should also make mention
of the fact that WITH ORDINALITY is not currently supported in that
case.
3). I think it would be good to have a more meaningful default name
for the new ordinality column, rather than "?column?". In many cases
the user might then choose to not bother giving it an alias. It could
simply be called ordinality by default, since that's non-reserved.
4). In gram.y, WITH_ORDINALITY should not be listed as an ordinary
keyword, but instead should be listed as a token below that (just
before WITH_TIME).
5). In plannodes.h, FunctionScan's new field should probably have a comment.
6). In parsenodes.h, the field added to RangeTblEntry is only valid
for function RTEs, so it should be moved to that group of fields and
renamed appropriately (unless you're expecting to extend it to other
RTE kinds in the future?). Logically then, the new check for
ordinality in inline_set_returning_function() should be moved so that
it is after the check that the RTE actually a function RTE, and in
addRangeTableEntryForFunction() the RTE's ordinality field should be
set at the start along with the other function-related fields.
7). In execnodes.h, the new fields added to FunctionScanState should
be documented in the comment block above.
Overall, as I said, I really like this feature, and I think it's not
far from being commitable.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 18, 2013 at 11:36:08AM +0100, Dean Rasheed wrote:
On 17 June 2013 06:33, David Fetter <david@fetter.org> wrote:
Next revision of the patch, now with more stability. Thanks, Andrew!
Rebased vs. git master.
Here's my review of the WITH ORDINALITY patch.
Overall I think that the patch is in good shape, and I think that this
will be a very useful new feature, so I'm keen to see this committed.All the basic stuff is OK --- the patch applies cleanly, compiles with
no warnings, and has appropriate docs and new regression tests which
pass. I also tested it fairly thoroughly myself, and I couldn't break
it. Everything worked as I expected, and it works nicely with LATERAL.I have a few minor comments that should be considered before passing
it on to a committer:1). In func.sgml, the new doc example "unnest WITH ORDINALITY" is
mislablled, since it it's not actually an example of unnest().
Fixed in patch attached.
Also it doesn't really belong in that code example block, which is
about generate_subscripts(). I think that there should probably be a
new sub-section starting at that point for WITH ORDINALITY. Perhaps
it should start with a brief paragraph explaining how WITH
ORDINALITY can be applied to functions in the FROM clause of a
query.
How's the attached?
[Actually it appears that WITH ORDINALITY works with non-SRF's too,
but that's less useful, so I think that the SRF section is probably
still the right place to document this]
As of this patch, that's now both in the SELECT docs and the SRF
section.
It might also be worth mentioning here that currently WITH ORDINALITY
is not supported for functions that return records.
Added.
In the code example itself, the prompt should be trimmed down to match
the previous examples.
Done.
2). In the SELECT docs, where function_name is documented, I would be
tempted to start a new paragraph for the sentence starting "If the
function has been defined as returning the record data type...", since
that's really a separate syntax. I think that should also make mention
of the fact that WITH ORDINALITY is not currently supported in that
case.
Done-ish. What do you think?
3). I think it would be good to have a more meaningful default name
for the new ordinality column, rather than "?column?". In many cases
the user might then choose to not bother giving it an alias. It could
simply be called ordinality by default, since that's non-reserved.
I don't think this needs doing, per spec. The column name needs to be
unique, and if someone happens to name an output column of a function,
"?column?", that's really not our problem.
4). In gram.y, WITH_ORDINALITY should not be listed as an ordinary
keyword, but instead should be listed as a token below that (just
before WITH_TIME).
Done.
5). In plannodes.h, FunctionScan's new field should probably have a comment.
Done.
6). In parsenodes.h, the field added to RangeTblEntry is only valid
for function RTEs, so it should be moved to that group of fields and
renamed appropriately (unless you're expecting to extend it to other
RTE kinds in the future?).
Nope, and done.
Logically then, the new check for ordinality in
inline_set_returning_function() should be moved so that it is after
the check that the RTE actually a function RTE, and in
addRangeTableEntryForFunction() the RTE's ordinality field should be
set at the start along with the other function-related fields.
We don't actually get to inline_set_returning_function unless it's a
function RTE.
7). In execnodes.h, the new fields added to FunctionScanState should
be documented in the comment block above.
Done.
Overall, as I said, I really like this feature, and I think it's not
far from being commitable.
Great! :)
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
ordinality_08.difftext/plain; charset=us-asciiDownload+1307-326
On 19 June 2013 04:11, David Fetter <david@fetter.org> wrote:
On Tue, Jun 18, 2013 at 11:36:08AM +0100, Dean Rasheed wrote:
On 17 June 2013 06:33, David Fetter <david@fetter.org> wrote:
Next revision of the patch, now with more stability. Thanks, Andrew!
Rebased vs. git master.
Here's my review of the WITH ORDINALITY patch.
Overall I think that the patch is in good shape, and I think that this
will be a very useful new feature, so I'm keen to see this committed.All the basic stuff is OK --- the patch applies cleanly, compiles with
no warnings, and has appropriate docs and new regression tests which
pass. I also tested it fairly thoroughly myself, and I couldn't break
it. Everything worked as I expected, and it works nicely with LATERAL.I have a few minor comments that should be considered before passing
it on to a committer:1). In func.sgml, the new doc example "unnest WITH ORDINALITY" is
mislablled, since it it's not actually an example of unnest().Fixed in patch attached.
Also it doesn't really belong in that code example block, which is
about generate_subscripts(). I think that there should probably be a
new sub-section starting at that point for WITH ORDINALITY. Perhaps
it should start with a brief paragraph explaining how WITH
ORDINALITY can be applied to functions in the FROM clause of a
query.How's the attached?
Looks good.
[Actually it appears that WITH ORDINALITY works with non-SRF's too,
but that's less useful, so I think that the SRF section is probably
still the right place to document this]As of this patch, that's now both in the SELECT docs and the SRF
section.It might also be worth mentioning here that currently WITH ORDINALITY
is not supported for functions that return records.Added.
In the code example itself, the prompt should be trimmed down to match
the previous examples.Done.
Oh, on closer inspection, the previous examples mostly don't show the
prompt at all, except for the last one. So perhaps it should be
removed from both those places.
2). In the SELECT docs, where function_name is documented, I would be
tempted to start a new paragraph for the sentence starting "If the
function has been defined as returning the record data type...", since
that's really a separate syntax. I think that should also make mention
of the fact that WITH ORDINALITY is not currently supported in that
case.Done-ish. What do you think?
Hmm, I fear that might have made it worse, because now it reads as if
functions that return records can't be used in the FROM clause at all
(at least if you don't read all the way to the end, which many people
don't). I think if you just take out this change:
Function calls can appear in the <literal>FROM</literal>
clause. (This is especially useful for functions that return
- result sets, but any function can be used.) This acts as
+ result sets, but any function except those that return
+ <literal>[SETOF] RECORD</literal> can be used.) This acts asthen what's left is OK.
3). I think it would be good to have a more meaningful default name
for the new ordinality column, rather than "?column?". In many cases
the user might then choose to not bother giving it an alias. It could
simply be called ordinality by default, since that's non-reserved.I don't think this needs doing, per spec. The column name needs to be
unique, and if someone happens to name an output column of a function,
"?column?", that's really not our problem.
I don't think the spec says anything about how the new column should
be named, so it's up to us, but I do think a sensible default would be
useful to save the user from having to give it an alias in many common
cases.
For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
would then produce a column that could be referred to in the rest of
the query as file.ordinality or simply ordinality. As it stands,
they'd have to write file."?column?", which is really ugly, so we're
effectively forcing them to supply an alias for this column every
time. I think it would be better if they only had to supply a name to
resolve name conflicts, or if they wanted a different name.
4). In gram.y, WITH_ORDINALITY should not be listed as an ordinary
keyword, but instead should be listed as a token below that (just
before WITH_TIME).Done.
5). In plannodes.h, FunctionScan's new field should probably have a comment.
Done.
6). In parsenodes.h, the field added to RangeTblEntry is only valid
for function RTEs, so it should be moved to that group of fields and
renamed appropriately (unless you're expecting to extend it to other
RTE kinds in the future?).Nope, and done.
Logically then, the new check for ordinality in
inline_set_returning_function() should be moved so that it is after
the check that the RTE actually a function RTE, and in
addRangeTableEntryForFunction() the RTE's ordinality field should be
set at the start along with the other function-related fields.We don't actually get to inline_set_returning_function unless it's a
function RTE.
OK, yes you're right.
7). In execnodes.h, the new fields added to FunctionScanState should
be documented in the comment block above.Done.
There's still a comment relating the old tupdesc field in the comment
block above. I think for consistency with the surrounding code, all
those comments should be in header comment block (except for the
NodeTag one).
Everything else looks good. I think we're now down to just a few minor
comment/doc issues, and the question of whether the new column should
have a better default name.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 19, 2013 at 01:03:42PM +0100, Dean Rasheed wrote:
On 19 June 2013 04:11, David Fetter <david@fetter.org> wrote:
On Tue, Jun 18, 2013 at 11:36:08AM +0100, Dean Rasheed wrote:
On 17 June 2013 06:33, David Fetter <david@fetter.org> wrote:
Next revision of the patch, now with more stability. Thanks, Andrew!
Rebased vs. git master.
Here's my review of the WITH ORDINALITY patch.
Overall I think that the patch is in good shape, and I think that this
will be a very useful new feature, so I'm keen to see this committed.All the basic stuff is OK --- the patch applies cleanly, compiles with
no warnings, and has appropriate docs and new regression tests which
pass. I also tested it fairly thoroughly myself, and I couldn't break
it. Everything worked as I expected, and it works nicely with LATERAL.I have a few minor comments that should be considered before passing
it on to a committer:1). In func.sgml, the new doc example "unnest WITH ORDINALITY" is
mislablled, since it it's not actually an example of unnest().Fixed in patch attached.
Also it doesn't really belong in that code example block, which is
about generate_subscripts(). I think that there should probably be a
new sub-section starting at that point for WITH ORDINALITY. Perhaps
it should start with a brief paragraph explaining how WITH
ORDINALITY can be applied to functions in the FROM clause of a
query.How's the attached?
Looks good.
Thanks!
In the code example itself, the prompt should be trimmed down to match
the previous examples.Done.
Oh, on closer inspection, the previous examples mostly don't show the
prompt at all, except for the last one. So perhaps it should be
removed from both those places.
Done.
2). In the SELECT docs, where function_name is documented, I would be
tempted to start a new paragraph for the sentence starting "If the
function has been defined as returning the record data type...", since
that's really a separate syntax. I think that should also make mention
of the fact that WITH ORDINALITY is not currently supported in that
case.Done-ish. What do you think?
Hmm, I fear that might have made it worse, because now it reads as if
functions that return records can't be used in the FROM clause at all
(at least if you don't read all the way to the end, which many people
don't). I think if you just take out this change:Function calls can appear in the <literal>FROM</literal> clause. (This is especially useful for functions that return - result sets, but any function can be used.) This acts as + result sets, but any function except those that return + <literal>[SETOF] RECORD</literal> can be used.) This acts asthen what's left is OK.
Done.
3). I think it would be good to have a more meaningful default name
for the new ordinality column, rather than "?column?". In many cases
the user might then choose to not bother giving it an alias. It could
simply be called ordinality by default, since that's non-reserved.I don't think this needs doing, per spec. The column name needs to be
unique, and if someone happens to name an output column of a function,
"?column?", that's really not our problem.I don't think the spec says anything about how the new column should
be named, so it's up to us, but I do think a sensible default would be
useful to save the user from having to give it an alias in many common
cases.For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
The spec is pretty specific about the "all or none" nature of naming
in the AS clause...unless we can figure out a way of getting around it
somehow.
7). In execnodes.h, the new fields added to FunctionScanState should
be documented in the comment block above.Done.
There's still a comment relating the old tupdesc field in the comment
block above. I think for consistency with the surrounding code, all
those comments should be in header comment block (except for the
NodeTag one).
Done.
Everything else looks good. I think we're now down to just a few minor
comment/doc issues, and the question of whether the new column should
have a better default name.
I'm weighing in on the side of a name that's like ?columnN? elsewhere
in the code, i.e. one that couldn't sanely be an actual column name,
whether in table, view, or SRF.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
ordinality_09.difftext/plain; charset=us-asciiDownload+1310-327
On 21 June 2013 06:54, David Fetter <david@fetter.org> wrote:
For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
The spec is pretty specific about the "all or none" nature of naming
in the AS clause...unless we can figure out a way of getting around it
somehow.
We already support and document the ability to provide a subset of the
columns in the alias. I didn't realise that was beyond the spec, but
since we already have it...
I'm weighing in on the side of a name that's like ?columnN? elsewhere
in the code, i.e. one that couldn't sanely be an actual column name,
whether in table, view, or SRF.
I don't think we need to be overly concerned with coming up with a
unique name for the column. Duplicate column names are fine, except if
the user wants to refer to them elsewhere in the query, in which case
they need to provide aliases to distinguish them, otherwise the query
won't be accepted.
I'd be happy if you just replaced "?column?" with ordinality in a
couple of places in your original patch.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 21 June 2013 08:02, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 06:54, David Fetter <david@fetter.org> wrote:
For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
The spec is pretty specific about the "all or none" nature of naming
in the AS clause...unless we can figure out a way of getting around it
somehow.We already support and document the ability to provide a subset of the
columns in the alias. I didn't realise that was beyond the spec, but
since we already have it...I'm weighing in on the side of a name that's like ?columnN? elsewhere
in the code, i.e. one that couldn't sanely be an actual column name,
whether in table, view, or SRF.I don't think we need to be overly concerned with coming up with a
unique name for the column. Duplicate column names are fine, except if
the user wants to refer to them elsewhere in the query, in which case
they need to provide aliases to distinguish them, otherwise the query
won't be accepted.I'd be happy if you just replaced "?column?" with ordinality in a
couple of places in your original patch.
Expanding on that, I think it's perfectly acceptable to allow
potentially duplicate column names in this context. For the majority
of simple queries the user wouldn't have to (and wouldn't feel
compelled to) supply aliases. Where there was ambiguity they would be
forced to do so, but people are already used to that.
If I write a simple query that selects from a single unnest() with or
without ordinality, I probably won't want to supply aliases.
If I select from 2 unnest()'s *without* ordinality, I already have to
provide aliases.
If I select from 2 SRF's functions with ordinality, I won't be too
surprised if I am also forced to provide aliases.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
OK, let's try to cover all the bases here in one go.
First, the spec definition of WITH ORDINALITY simply says that the
column name in the result is not equivalent to any other identifier in
the same <table primary> (including the <correlation name>). It is
clear that the intention of the spec is that any non-positional
reference to this column (which is defined as being positionally last)
requires an alias at some point, whether directly attached to the
<table primary> or at an outer level.
Second, all the documentation I've looked at for other databases that
implement this feature (such as DB2, Teradata, etc.) takes it for
granted that the user must always supply an alias, even though the
syntax does not actually require one. None of the ones I've seen
suggest that the ordinality column has a useful or consistent name if
no alias is supplied.
So, while clearly there's nothing stopping us from going beyond the
spec and using a column name that people can refer to without needing
an alias, it would be a significant divergence from common practice in
other dbs. (iirc, it was my suggestion to David to use "?column?" in
the first place for this reason.)
So as I see it the options are:
1. Stick with "?column?" as a warning flag that you're not supposed to
be using this without aliasing it to something.
2. Use some other fixed name like "ordinality" simply to allow people
to do things like select ... from unnest(x) with ordinality; without
having to bother to provide an alias, simply as a convenience, without
regard for consistency with others. (This will result in a duplicate
name if "x" is of a composite type containing a column called
"ordinality", so the caller will have to provide an alias in that
specific case or get an ambiguous reference error. Similarly if using
some other SRF which defines its own return column names.)
3. Generate an actually unique name (probably pointless)
4. Something else I haven't thought of.
My vote remains with option 1 here; I don't think users should be
encouraged to assume that the ordinality column will have a known
name.
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/23/2013 08:00 PM, Andrew Gierth wrote:
OK, let's try to cover all the bases here in one go.
1. Stick with "?column?" as a warning flag that you're not supposed to
be using this without aliasing it to something.
How do I actually supply an alias which covers both columns? What does
that look like, syntactically?
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM48cbb5d38c74338510be8883854317e1c014e2a67fb31b00f653a6cccc0246a3ab727b58a599dd77861b5b3c6fc7ff45@asav-1.01.com
On 21 June 2013 08:31, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 08:02, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 06:54, David Fetter <david@fetter.org> wrote:
For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
The spec is pretty specific about the "all or none" nature of naming
in the AS clause...unless we can figure out a way of getting around it
somehow.We already support and document the ability to provide a subset of the
columns in the alias. I didn't realise that was beyond the spec, but
since we already have it...I'm weighing in on the side of a name that's like ?columnN? elsewhere
in the code, i.e. one that couldn't sanely be an actual column name,
whether in table, view, or SRF.I don't think we need to be overly concerned with coming up with a
unique name for the column. Duplicate column names are fine, except if
the user wants to refer to them elsewhere in the query, in which case
they need to provide aliases to distinguish them, otherwise the query
won't be accepted.I'd be happy if you just replaced "?column?" with ordinality in a
couple of places in your original patch.Expanding on that, I think it's perfectly acceptable to allow
potentially duplicate column names in this context. For the majority
of simple queries the user wouldn't have to (and wouldn't feel
compelled to) supply aliases. Where there was ambiguity they would be
forced to do so, but people are already used to that.If I write a simple query that selects from a single unnest() with or
without ordinality, I probably won't want to supply aliases.If I select from 2 unnest()'s *without* ordinality, I already have to
provide aliases.If I select from 2 SRF's functions with ordinality, I won't be too
surprised if I am also forced to provide aliases.
No one else has expressed an opinion about the naming of the new
column. All other review comments have been addressed, and the patch
looks to be in good shape, so I'm marking this as ready for committer.
IMO it's a very useful piece of new functionality. Good job!
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 24 June 2013 04:29, Josh Berkus <josh@agliodbs.com> wrote:
On 06/23/2013 08:00 PM, Andrew Gierth wrote:
OK, let's try to cover all the bases here in one go.
1. Stick with "?column?" as a warning flag that you're not supposed to
be using this without aliasing it to something.How do I actually supply an alias which covers both columns? What does
that look like, syntactically?
There are a number of possible syntaxes:
SELECT unnest, "?column?" FROM unnest(ARRAY['x','y']) WITH ORDINALITY;
or
SELECT unnest.unnest, unnest."?column?" FROM unnest(ARRAY['x','y'])
WITH ORDINALITY;
unnest | ?column?
--------+----------
x | 1
y | 2
(2 rows)
SELECT t, "?column?" FROM unnest(ARRAY['x','y']) WITH ORDINALITY AS t;
or
SELECT t.t, t."?column?" FROM unnest(ARRAY['x','y']) WITH ORDINALITY AS t;
t | ?column?
---+----------
x | 1
y | 2
(2 rows)
SELECT val, "?column?" FROM unnest(ARRAY['x','y']) WITH ORDINALITY AS t(val);
or
SELECT t.val, t."?column?" FROM unnest(ARRAY['x','y']) WITH ORDINALITY
AS t(val);
val | ?column?
-----+----------
x | 1
y | 2
(2 rows)
SELECT val, ord FROM unnest(ARRAY['x','y']) WITH ORDINALITY AS t(val, ord);
or
SELECT t.val, t.ord FROM unnest(ARRAY['x','y']) WITH ORDINALITY AS t(val, ord);
val | ord
-----+-----
x | 1
y | 2
(2 rows)
My suggestion was to replace "?column?" with ordinality wherever it
appears above, for the user's convenience, but so far more people
prefer "?column?" as a way of indicating that you're supposed to
provide an alias for the column.
If that's what people prefer, I don't mind --- it's still going to be
a very handy new feature.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Folks,
(the below was already discussed on IRC)
Leaving names aside on this patch, I'm wondering about a piece of
functionality I have with the current unnest() and with the
unnest_ordinality()[1] extension: namely, the ability to unnest several
arrays "in parallel" by using unnest() in the target list.
For example, given the table:
lotsarrays (
id serial PK,
arr1 int[],
arr2 numeric[],
arr3 boolean[]
)
I can currently do:
SELECT id,
unnest(arr1) as arr1,
unnest(arr2) as arr2,
unnest(arr3) as arr3
FROM lotsarrays;
... and if arr1, 2 and 3 are exactly the same length, this creates a
coordinated dataset. I can even use the unnest_ordinality() extension
function to get the ordinality of this combined dataset:
SELECT id,
(unnest_ordinality(arr1)).element_number as array_index,
unnest(arr1) as arr1,
unnest(arr2) as arr2,
unnest(arr3) as arr3
FROM lotsarrays;
There are reasons why this will be complicated to implement WITH
ORDINALITY; DF, Andrew and I discussed them on IRC. So allowing WITH
ORDINALITY in the target list is a TODO, either for later in 9.4
development, or for 9.5.
So, this isn't stopping the patch; I just want a TODO for "implement
WITH ORDINALITY in the target list for SRFs".
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WMc42dd597d1c041b285fe9b47096a8c3b2aade9fb3e2bfb1ef418e0ef2e790ec632ffc0c2130642cff0aff67d2de72996@asav-2.01.com
Josh Berkus <josh@agliodbs.com> writes:
... and if arr1, 2 and 3 are exactly the same length, this creates a
coordinated dataset. I can even use the unnest_ordinality() extension
function to get the ordinality of this combined dataset:
SELECT id,
(unnest_ordinality(arr1)).element_number as array_index,
unnest(arr1) as arr1,
unnest(arr2) as arr2,
unnest(arr3) as arr3
FROM lotsarrays;
There are reasons why this will be complicated to implement WITH
ORDINALITY; DF, Andrew and I discussed them on IRC. So allowing WITH
ORDINALITY in the target list is a TODO, either for later in 9.4
development, or for 9.5.
Some of the rest of us would like to hear those reasons, because my
immediate reaction is that the patch must be broken by design. WITH
ORDINALITY should not be needing to mess with the fundamental evaluation
semantics of SRFs, but it sure sounds like it is doing so if that case
doesn't work as expected.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 26 June 2013 01:22, Josh Berkus <josh@agliodbs.com> wrote:
Folks,
(the below was already discussed on IRC)
Leaving names aside on this patch, I'm wondering about a piece of
functionality I have with the current unnest() and with the
unnest_ordinality()[1] extension: namely, the ability to unnest several
arrays "in parallel" by using unnest() in the target list.For example, given the table:
lotsarrays (
id serial PK,
arr1 int[],
arr2 numeric[],
arr3 boolean[]
)I can currently do:
SELECT id,
unnest(arr1) as arr1,
unnest(arr2) as arr2,
unnest(arr3) as arr3
FROM lotsarrays;... and if arr1, 2 and 3 are exactly the same length, this creates a
coordinated dataset. I can even use the unnest_ordinality() extension
function to get the ordinality of this combined dataset:SELECT id,
(unnest_ordinality(arr1)).element_number as array_index,
unnest(arr1) as arr1,
unnest(arr2) as arr2,
unnest(arr3) as arr3
FROM lotsarrays;There are reasons why this will be complicated to implement WITH
ORDINALITY; DF, Andrew and I discussed them on IRC. So allowing WITH
ORDINALITY in the target list is a TODO, either for later in 9.4
development, or for 9.5.So, this isn't stopping the patch; I just want a TODO for "implement
WITH ORDINALITY in the target list for SRFs".
So if I'm understanding correctly, your issue is that WITH ORDINALITY
is currently only accepted on SRFs in the FROM list, not that it isn't
working as expected in any way. I have no objection to adding a TODO
item to extend it, but note that the restriction is trivial to work
around:
CREATE TABLE lotsarrays
(
id serial primary key,
arr1 int[],
arr2 numeric[],
arr3 boolean[]
);
INSERT INTO lotsarrays(arr1, arr2, arr3) VALUES
(ARRAY[1,2], ARRAY[1.1, 2.2], ARRAY[true, false]),
(ARRAY[10,20,30], ARRAY[10.1, 20.2, 30.3], ARRAY[true, false, true]);
CREATE OR REPLACE FUNCTION unnest_ordinality(anyarray)
RETURNS TABLE(element_number bigint, element anyelement) AS
$$
SELECT ord, elt FROM unnest($1) WITH ORDINALITY AS t(elt, ord)
$$
LANGUAGE sql STRICT IMMUTABLE;
SELECT id,
(unnest_ordinality(arr1)).element_number as array_index,
unnest(arr1) as arr1,
unnest(arr2) as arr2,
unnest(arr3) as arr3
FROM lotsarrays;
id | array_index | arr1 | arr2 | arr3
----+-------------+------+------+------
1 | 1 | 1 | 1.1 | t
1 | 2 | 2 | 2.2 | f
2 | 1 | 10 | 10.1 | t
2 | 2 | 20 | 20.2 | f
2 | 3 | 30 | 30.3 | t
(5 rows)
Personally I'm not a fan of SRFs in the select list, especially not
multiple SRFs there, since the results are hard to deal with if they
return differing numbers of rows. So I would tend to write this as a
LATERAL FULL join on the ordinality columns:
SELECT id,
COALESCE(u1.ord, u2.ord, u3.ord) AS array_index,
u1.arr1, u2.arr2, u3.arr3
FROM lotsarrays,
unnest(arr1) WITH ORDINALITY AS u1(arr1, ord)
FULL JOIN unnest(arr2) WITH ORDINALITY AS u2(arr2, ord) ON u2.ord = u1.ord
FULL JOIN unnest(arr3) WITH ORDINALITY AS u3(arr3, ord) ON u3.ord =
COALESCE(u1.ord, u2.ord);
Either way, I think the WITH ORDINALITY patch is working as expected.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 24, 2013 at 03:04:04PM +0100, Dean Rasheed wrote:
On 21 June 2013 08:31, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 08:02, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 06:54, David Fetter <david@fetter.org> wrote:
For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
The spec is pretty specific about the "all or none" nature of naming
in the AS clause...unless we can figure out a way of getting around it
somehow.We already support and document the ability to provide a subset of the
columns in the alias. I didn't realise that was beyond the spec, but
since we already have it...I'm weighing in on the side of a name that's like ?columnN? elsewhere
in the code, i.e. one that couldn't sanely be an actual column name,
whether in table, view, or SRF.I don't think we need to be overly concerned with coming up with a
unique name for the column. Duplicate column names are fine, except if
the user wants to refer to them elsewhere in the query, in which case
they need to provide aliases to distinguish them, otherwise the query
won't be accepted.I'd be happy if you just replaced "?column?" with ordinality in a
couple of places in your original patch.Expanding on that, I think it's perfectly acceptable to allow
potentially duplicate column names in this context. For the majority
of simple queries the user wouldn't have to (and wouldn't feel
compelled to) supply aliases. Where there was ambiguity they would be
forced to do so, but people are already used to that.If I write a simple query that selects from a single unnest() with or
without ordinality, I probably won't want to supply aliases.If I select from 2 unnest()'s *without* ordinality, I already have to
provide aliases.If I select from 2 SRF's functions with ordinality, I won't be too
surprised if I am also forced to provide aliases.No one else has expressed an opinion about the naming of the new
column. All other review comments have been addressed, and the patch
looks to be in good shape, so I'm marking this as ready for committer.IMO it's a very useful piece of new functionality. Good job!
Thanks!
Please find attach another rev of the patch which reflects the
de-reserving of OVER.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
ordinality_10.difftext/plain; charset=us-asciiDownload+1310-327
On Sun, Jun 30, 2013 at 06:00:35PM -0700, David Fetter wrote:
On Mon, Jun 24, 2013 at 03:04:04PM +0100, Dean Rasheed wrote:
On 21 June 2013 08:31, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 08:02, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 June 2013 06:54, David Fetter <david@fetter.org> wrote:
For example "SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS file"
The spec is pretty specific about the "all or none" nature of naming
in the AS clause...unless we can figure out a way of getting around it
somehow.We already support and document the ability to provide a subset of the
columns in the alias. I didn't realise that was beyond the spec, but
since we already have it...I'm weighing in on the side of a name that's like ?columnN? elsewhere
in the code, i.e. one that couldn't sanely be an actual column name,
whether in table, view, or SRF.I don't think we need to be overly concerned with coming up with a
unique name for the column. Duplicate column names are fine, except if
the user wants to refer to them elsewhere in the query, in which case
they need to provide aliases to distinguish them, otherwise the query
won't be accepted.I'd be happy if you just replaced "?column?" with ordinality in a
couple of places in your original patch.Expanding on that, I think it's perfectly acceptable to allow
potentially duplicate column names in this context. For the majority
of simple queries the user wouldn't have to (and wouldn't feel
compelled to) supply aliases. Where there was ambiguity they would be
forced to do so, but people are already used to that.If I write a simple query that selects from a single unnest() with or
without ordinality, I probably won't want to supply aliases.If I select from 2 unnest()'s *without* ordinality, I already have to
provide aliases.If I select from 2 SRF's functions with ordinality, I won't be too
surprised if I am also forced to provide aliases.No one else has expressed an opinion about the naming of the new
column. All other review comments have been addressed, and the patch
looks to be in good shape, so I'm marking this as ready for committer.IMO it's a very useful piece of new functionality. Good job!
Thanks!
Please find attach another rev of the patch which reflects the
de-reserving of OVER.
Patch re-jiggered for recent changes to master.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
ordinality_11.difftext/plain; charset=us-asciiDownload+1310-327
On 4 July 2013 00:08, David Fetter <david@fetter.org> wrote:
Patch re-jiggered for recent changes to master.
I re-validated this, and it all still looks good, so still ready for
committer IMO.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/04/2013 10:15 AM, Dean Rasheed wrote:
On 4 July 2013 00:08, David Fetter <david@fetter.org> wrote:
Patch re-jiggered for recent changes to master.
I re-validated this, and it all still looks good, so still ready for
committer IMO.
I tried to check this out, too and "make check" fails with the
following. I have not looked into the cause.
$ cat /home/vik/postgresql/git/src/test/regress/log/initdb.log
Running in noclean mode. Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user "vik".
This user must also own the server process.
The database cluster will be initialized with locales
COLLATE: en_US.UTF-8
CTYPE: en_US.UTF-8
MESSAGES: C
MONETARY: en_US.UTF-8
NUMERIC: en_US.UTF-8
TIME: en_US.UTF-8
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".
Data page checksums are disabled.
creating directory
/home/vik/postgresql/git/src/test/regress/./tmp_check/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
creating configuration files ... ok
creating template1 database in
/home/vik/postgresql/git/src/test/regress/./tmp_check/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... FATAL: role with OID 256 does not exist
STATEMENT: DELETE FROM pg_depend;
child process exited with exit code 1
initdb: data directory
"/home/vik/postgresql/git/src/test/regress/./tmp_check/data" not removed
at user's request
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/05/2013 04:51 PM, Vik Fearing wrote:
I tried to check this out, too and "make check" fails with the
following. I have not looked into the cause.
Running ./configure again fixed it. Sorry for the noise.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jul 05, 2013 at 04:58:30PM +0200, Vik Fearing wrote:
On 07/05/2013 04:51 PM, Vik Fearing wrote:
I tried to check this out, too and "make check" fails with the
following. I have not looked into the cause.Running ./configure again fixed it. Sorry for the noise.
If I had a nickel for every apparent failure of this nature, I'd never
need to work again.
Thanks for checking :)
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 26, 2013 at 2:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Some of the rest of us would like to hear those reasons, because my
immediate reaction is that the patch must be broken by design. WITH
ORDINALITY should not be needing to mess with the fundamental evaluation
semantics of SRFs, but it sure sounds like it is doing so if that case
doesn't work as expected.
As Dan pointed out later the patch is not affecting whether this case
works as expected. Only that since you can only use WITH ORDINALITY on
SRFs in the FROM list and not in the target list if you want to use it
you have to rewrite this query to put the SRFs in the FROM list.
I think we're ok with that since SRFs in the target list are something
that already work kind of strangely and probably we would rather get
rid of rather than work to extend. It would be hard to work ordinality
into the grammar in the middle of the target list let alone figure out
how to implement it.
My only reservation with this patch is whether the WITH_ORDINALITY
parser hack is the way we want to go. The precedent was already set
with WITH TIME ZONE though and I think this was the best option. The
worst failure I can come up with is this which doesn't seem like a
huge problem:
postgres=# with o as (select 1 ) select * from o;
?column?
----------
1
(1 row)
postgres=# with ordinality as (select 1 ) select * from ordinality;
ERROR: syntax error at or near "ordinality"
LINE 1: with ordinality as (select 1 ) select * from ordinality;
^
--
greg
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
