From: | Joseph Koshakow <koshy44(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Jacob Champion <jchampion(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: DecodeInterval fixes |
Date: | 2023-07-08 17:18:32 |
Message-ID: | CAAvxfHeHXQqiUoZyw4ODx4CULg+Had_xw0pFA5_YOVhXamEnqQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Jacob Champion <jchampion(at)timescale(dot)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
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Fix-interval-decode-handling-of-invalid-intervals.patch | text/x-patch | 5.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2023-07-08 17:28:43 | Re: Check lateral references within PHVs for memoize cache keys |
Previous Message | Tom Lane | 2023-07-08 17:07:21 | Re: psql: Add role's membership options to the \du+ command |