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

On 19 June 2013 04:11, David Fetter <david(at)fetter(dot)org> wrote:
> On Tue, Jun 18, 2013 at 11:36:08AM +0100, Dean Rasheed wrote:
>> On 17 June 2013 06:33, David Fetter <david(at)fetter(dot)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 as

then 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