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

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