Hello Alexander,
On Thu, January 4, 2018 4:36 pm, Alexander Korotkov wrote:
> On Fri, Dec 8, 2017 at 4:06 PM, Alexander Korotkov <
> [email protected]> wrote:
>
>> Thank you for pointing that. Sure, both cases are better. I've added
>> second case as well as comments. Patch is attached.
I had a quick look, this isn't a full review, but a few things struck me
on a read through the diff:
There are quite a few places where lines are broken like so:
+
ExecIncrementalSortInitializeWorker((IncrementalSortState *) planstate,
+
pwcxt);
Or like this:
+ result = (PlanState *) ExecInitIncrementalSort(
+
(IncrementalSort *) node, estate, eflags);
e.g. a param is on the next line, but aligned to the very same place where
it would be w/o the linebreak. Or is this just some sort of artefact
because I viewed the diff with tabspacing = 8?
I'd fix the grammar here:
+ * Incremental sort is specially optimized kind of multikey sort
when
+ * input is already presorted by prefix of required keys list.
Like so:
"Incremental sort is a specially optimized kind of multikey sort used when
the input is already presorted by a prefix of the required keys list."
+ * Consider following example. We have input tuples consisting
from
"Consider the following example: We have ..."
+ * In incremental sort case we also have to cost to detect sort
groups.
"we also have to cost the detection of sort groups."
"+ * It turns out into extra copy and comparison for each tuple."
"This turns out to be one extra copy and comparison per tuple."
+ "Portions Copyright (c) 1996-2017"
Should probably be 2018 now - time flies fast :)
return_value = _readMaterial();
else if (MATCH("SORT", 4))
return_value = _readSort();
+ else if (MATCH("INCREMENTALSORT", 7))
+ return_value = _readIncrementalSort();
else if (MATCH("GROUP", 5))
return_value = _readGroup();
I think the ", 7" here is left-over from when it was named "INCSORT", and
it should be MATCH("INCREMENTALSORT", 15)), shouldn't it?
+ space, fase
when it's value for in-memory
typo: "space, false when ..."
+ bool cmp;
+ cmp = cmpSortSkipCols(node, node->sampleSlot, slot);
+
+ if (cmp)
In the above, the variable cmp could be optimized away with:
+ if (cmpSortSkipCols(node, node->sampleSlot, slot))
(not sure if modern compilers won't do this, anway, though)
+typedef struct IncrementalSortState
+{
+ ScanState ss; /* its first field is
NodeTag */
+ bool bounded; /* is the result set
bounded? */
+ int64 bound; /* if bounded, how many
tuples are needed */
If I'm not wrong, the layout of the struct will include quite a bit of
padding on 64 bit due to the mixing of bool and int64, maybe it would be
better to sort the fields differently, e.g. pack 4 or 8 bools together?
Not sure if that makes much of a difference, though.
That's all for now :)
Thank you for your work,
Tels
