> On 26 Jun 2018, at 17:11, Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru> wrote:
> I took a look at the patch. It applies and compiles, the tests pass.
Thanks for reviewing, and apologies for the slow response.
> Some thoughts about the code:
>
> * Postgres lists cache their lengths, so you don't need uniqueLen.
Good point, fixed.
> * Use an array of WindowClauseSortNode's instead of a list. It's more suitable here because you are going to sort
(qsort_listcreates a temporary array).
I was thinking about that, but opted for code simplicity with a List approach.
The required size of the array isn’t known ahead of time, so it must either
potentially overallocate to the upper bound of root->parse->windowClause or use
heuristics and risk reallocating when growing, neither of which is terribly
appealing. Do you have any suggestions or preferences?
> * Reversing the sorted list looks more confusing to me than just sorting it in the proper order right away. A qsort()
comparatorreturning negative means "left goes before right", but here it is effectively the other way around.
Changed.
> * There is a function named make_pathkeys_for_window that makes a list of canonical pathkeys given a window clause.
Usingthis function to give you the pathkeys, and then comparing them, would be more future-proof in case we ever start
usinghashing for windowing. Moreover, the canonical pathkeys can be compared with pointer comparison which is much
fasterthan equal(). Still, I'm not sure whether it's going to be convenient to use in this case, or even whether it is
aright thing to do. What do you think?
That’s an interesting thought, one that didn’t occur to me while hacking. I’m
not sure whether is would be wise/clean to overload with this functionality
though.
Attached updated version also adds a testcase that was clearly missing from the
previous version and an updated window.out.
cheers ./daniel