Re: Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls - Mailing list pgsql-hackers
From | Alvaro Herrera |
---|---|
Subject | Re: Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls |
Date | |
Msg-id | 20131010213418.GH4825@eldon.alvh.no-ip.org Whole thread Raw |
In response to | Re: Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls (Nicholas White <n.j.white@gmail.com>) |
List | pgsql-hackers |
We have this block: + { + /* + * This is the window we want - but we have to tweak the + * definition slightly (e.g. to support the IGNORE NULLS frame + * option) as we're not using the default (i.e. parent) frame + * options. + * + * We'll create a 'child' (using refname to inherit everything + * from the parent) that just overrides the frame options + * (assuming it doesn't already exist): + */ + WindowDef *clone = makeNode(WindowDef); ... then it goes to populate the clone. When this is done, we use the clone to walk the list of existing WindowDefs, and if there's a match we free this one and use that one. Wouldn't it be better to walk the existing array first looking for a match, and only create a clone if none is found? This would avoid the memory leak problems; I originally pointed out that you're leaking the bits created by this makeNode() call above, but now that I look at it again, I think you're also leaking the bits created by the two copyObject() calls to create the clone. It appears to me that it's simpler to not allocate any memory in the first place, unless necessary. Also, in parsenodes.h, you had the [MANDATORY] and such tags. Three things about that: 1) it looks a lot uglier than the original, so how about the modified version below? and 2) what does "MANDATORY value of NULL" means? Maybe you mean "MANDATORY value or NULL" instead? 3) Exactly what case does the "in this case" phrase refer to? I think the comment should be more explicit. Also, I think this should be its own paragraph instead of being mixed with the "For entries in a" paragraph. /** WindowDef - raw representation of WINDOW and OVER clauses** For entries in a WINDOW list, "name" is the window name beingdefined.* For OVER clauses, we use "name" for the "OVER window" syntax, or "refname"* for the "OVER (window)" syntax,which is subtly different --- the latter* implies overriding the window frame clause.* * In this case, the per-fieldindicators determine what the semantics* are:* [V]irtual* If NULL, then the parent's (refname)value is used.* [M]andatory* Never inherited from the parent, so must be specified; may be NULL.* [S]uper* Always inherited from parent, any local version ignored.*/ typedef struct WindowDef {NodeTag type;char *name; /* [M] window's own name */char *refname; /* [M] referencedwindow name, if any */List *partitionClause; /* [V] PARTITION BY expression list */List *orderClause; /* [M] ORDER BY (list of SortBy) */int frameOptions; /* [M] frame_clause options, see below */Node *startOffset; /* [M] expression for starting bound, if any */Node *endOffset; /* [M] expressionfor ending bound, if any */int location; /* parse location, or -1 if none/unknown */ } WindowDef; In gram.y there are some spurious whitespaces at end-of-line. You should be able to see them with git diff --check. (I don't think we support running pgindent on .y files, which would have otherwise cleaned this up.) A style issue. You have this: + /* + * We can process a constant offset much more efficiently; initially + * we'll scan through the first <offset> non-null rows, and store that + * index. On subsequent rows we'll decide whether to push that index + * forwards to the next non-null value, or just return it again. + */ + leadlag_const_context *context = WinGetPartitionLocalMemory( + winobj, + sizeof(leadlag_const_context)); + int count_forward = 0; I think it'd be better to put the declarations above the comment, and assignment to "context" below the comment. This way, the indentation of the assignment is not so odd. So it'd look like + leadlag_const_context *context; + int count_forward = 0; + + /* + * We can process a constant offset much more efficiently; initially + * we'll scan through the first <offset> non-null rows, and store that + * index. On subsequent rows we'll decide whether to push that index + * forwards to the next non-null value, or just return it again. + */ + context = WinGetPartitionLocalMemory(winobj, + sizeof(leadlag_const_context)); And a final style comment. You have a block like this: if (ignore_nulls && !const_offset) { long block; } else if (ignore_nulls /* && const_offset */) { another long block; } else { more stuff; } I think this looks better like this, even if it causes an extra level of indentation: if (ignore_nulls) { if (const_offset) { some stuff; } else { more; } } else { the third block; } Finally, I'm not really sure about the column you added to the regression tests table. It looks way too artificial; I mean the column name even states what test is going to use that data (respect=gorilla? uhm). I'm not sure what's a better option; maybe if you just named the column "favorite_pet" or something like that, it would appear less random. Maybe it'd be better to just create your own table for this. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
pgsql-hackers by date: