On Sat, 2023-07-08 at 13:18 -0400, Joseph Koshakow wrote:
> Jacob Champion <jchampion@timescale.com> writes:
> > Hi Joe, here's a partial review:
>
> Thanks so much for the review!
>
> > I'm new to this code, but I agree that the use of `type` and the
> > lookahead are not particularly obvious/intuitive. At the very
> > least,
> > they'd need some more explanation in the code. Your boolean flag
> > idea
> > sounds reasonable, though.
>
> I've updated the patch with the boolean flag idea. I think it's a
> bit cleaner and more readable.
>
> > > There is one more problem I noticed, but didn't fix. We allow
> > > multiple
> > > "@" to be sprinkled anywhere in the input, even though the docs
> > > [0]
> > > only allow it to appear at the beginning of the input.
> >
> > (No particular opinion on this.)
>
> I looked into this a bit. The reason this works is because the date
> time lexer filters out all punctuation. That's what allows us to
> parse
> things like `SELECT date 'January 8, 1999';`. It's probably not worth
> trying to be smarter about what punctuation we allow where, at least
> for now. Maybe in the future we can exclude "@" from the punctuation
> that get's filtered out.
>
> > It looks like this patch needs a rebase for the CI, too, but there
> > are
> > no conflicts.
>
> The attached patch is rebased against master.
>
> Thanks,
> Joe Koshakow
Apologies, I'm posting a little behind the curve here. My initial
thoughts on the original patch mirrored Jacob's re 1 and 2 - that it looked
good, did we need to consider the modified ecpg copy (which has been
answered by Tom). I didn't have have any strong thought re 3) or the '@'.
The updated patch looks good to me. Seems a little clearer/cleaner.
Thanks,
Reid