Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

From: Nicholas White <n(dot)j(dot)white(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls
Date: 2013-06-18 22:27:32
Message-ID: CA+=vxNY0FcOZ=YESGjp4BYpGrjSL8oDChtCMqqrMD4DaVU5vpQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thanks for the reviews. I've attached a revised patch that has the lexer
refactoring Alvaro mentions (arbitarily using a goto rather than a bool
flag) and uses Jeff's idea of just storing the index of the last non-null
value (if there is one) in the window function's context (and not the Datum
itself).

However, Robert's right that SELECT ... ORDER BY respect NULLS LAST will
now fail. An earlier iteration of the patch had RESPECT and IGNORE as
reserved, but that would have broken tables with columns called "respect"
(etc.), which the current version allows. Is this backwards incompatibility
acceptable? If not, maybe I should try doing a two-token lookahead in the
token-aggregating code between the lexer and the parser (i.e. make a
RESPECT_NULLS token out of a sequence of RESPECT NULLS OVER tokens,
remembering to replace the OVER token)? Or what about adding an %expect
statement to the Bison grammar, confirming that the shift / reduce
conflicts caused by using the RESPECT, IGNORE & NULL_P tokens the in
out_clause rule are OK?

Thanks -

Nick

Attachment Content-Type Size
lead-lag-ignore-nulls.patch application/octet-stream 19.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message MauMau 2013-06-18 22:40:09 Re: Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
Previous Message Fujii Masao 2013-06-18 21:57:00 Re: SET work_mem = '1TB';