On Sat, 2023-07-08 at 13:18 -0400, Joseph Koshakow wrote: > Jacob Champion <jchamp...@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
