> On 26 Jun 2018, at 17:11, Alexander Kuzmenkov <a.kuzmen...@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_list creates 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() comparator returning 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. Using this function to give you the
> pathkeys, and then comparing them, would be more future-proof in case we ever
> start using hashing for windowing. Moreover, the canonical pathkeys can be
> compared with pointer comparison which is much faster than equal(). Still,
> I'm not sure whether it's going to be convenient to use in this case, or even
> whether it is a right 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
window_prefix_sort-v3.patch
Description: Binary data
