Re: [PATCH] explain sortorder

Hello Heikki,

abbreviated version:
Sorry, the problem is only the unhandy patch text format, not different opinions how to proceed.

Long version:
The v7 patch file already addressed your suggestions,
but the file contained serveral (old) local commits,
the new ones at the end of the patch text/file.

v7.1 is attached and addresses this issue providing a clean patch file.

V8 will - as mentioned - add missing docs and regression tests,
Mike suggested.

VlG-Arne & Marius

---
Marius Timmer
Zentrum für Informationsverarbeitung
Westfälische Wilhelms-Universität Münster
Einsteinstraße 60

mtimm_01(at)uni-muenster(dot)de

Am 13.01.2015 um 18:52 schrieb Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>:

> On 01/13/2015 06:04 PM, Timmer, Marius wrote:
>> -malloc() (StringInfo is used as suggested now).
>
> There really shouldn't be any snprintf() calls in the patch, when StringInfo is used correctly...
>
>> @@ -1187,6 +1187,7 @@ explain (verbose, costs off) select * from matest0 order by 1-id;
>> Sort
>> Output: matest0.id, matest0.name, ((1 - matest0.id))
>> Sort Key: ((1 - matest0.id))
>> + Sort Order: ASC NULLS LAST
>> -> Result
>> Output: matest0.id, matest0.name, (1 - matest0.id)
>> -> Append
>
> This patch isn't going to be committed with this output format. Please change per my suggestion earlier:
>
>> I don't like this output. If there are a lot of sort keys, it gets
>> difficult to match the right ASC/DESC element to the sort key it applies
>> to. (Also, there seems to be double-spaces in the list)
>>
>> I would suggest just adding the information to the Sort Key line. As
>> long as you don't print the modifiers when they are defaults (ASC and
>> NULLS LAST), we could print the information even in non-VERBOSE mode. So
>> it would look something like:
>>
>> Sort Key: sortordertest.n1 NULLS FIRST, sortordertest.n2 DESC
>
> Or if you don't agree with that, explain why.
>
> - Heikki
>