Re: Review: UNNEST (and other functions) WITH ORDINALITY

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 as
>
> then 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