Hi Joe, here's a partial review: On Sun, Apr 9, 2023 at 5:44 PM Joseph Koshakow <kosh...@gmail.com> wrote: > 1) Removes dead code for handling unit type RESERVE.
Looks good. From a quick skim it looks like the ECPG copy of this code (ecpg/pgtypeslib/interval.c) might need to be updated as well? > 2) Restrict the unit "ago" to only appear at the end of the > interval. According to the docs [0], this is the only valid place to > put it, but we allowed it multiple times at any point in the input. Also looks reasonable to me. (Same note re: ECPG.) > 3) Error when the user has multiple consecutive units or a unit without > an accompanying value. I spent a lot of time trying to come up with > robust ways to detect this and ultimately settled on using the "type" > field. I'm not entirely happy with this approach, because it involves > having to look ahead to the next field in a couple of places. The other > approach I was considering was to introduce a boolean flag called > "unhandled_unit". After parsing a unit it would be set to true, after > applying the unit to a number it would be set to false. If it was true > right before parsing a unit, then we would error. Please let me know > if you have any suggestions here. 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. > 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.) It looks like this patch needs a rebase for the CI, too, but there are no conflicts. Thanks! --Jacob
