On Mon, Jan 15, 2024 at 7:41 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
> Attached are two small fixup patches for your patch set. > Thanks, Peter. > > In the first one, I simplified the grammar for the .decimal() method. > It seemed a bit overkill to build a whole list structure when all we > need are 0, 1, or 2 arguments. > Agree. I added unary '+' and '-' support as well and thus thought of having separate rules altogether rather than folding those in. > Per SQL standard, the precision and scale arguments are unsigned > integers, so unary plus and minus signs are not supported. So my patch > removes that support, but I didn't adjust the regression tests for that. > However, PostgreSQL numeric casting does support a negative scale. Here is an example: # select '12345'::numeric(4,-2); numeric --------- 12300 (1 row) And thus thought of supporting those. Do we want this JSON item method to behave differently here? > > Also note that in your 0002 patch, the datetime precision is similarly > unsigned, so that's consistent. > > By the way, in your 0002 patch, don't see the need for the separate > datetime_method grammar rule. You can fold that into accessor_op. > Sure. > > Overall, I think it would be better if you combined all three of these > patches into one. Right now, you have arranged these as incremental > features, and as a result of that, the additions to the JsonPathItemType > enum and the grammar keywords etc. are ordered in the way you worked on > these features, I guess. It would be good to maintain a bit of sanity > to put all of this together and order all the enums and everything else > for example in the order they are in the sql_features.txt file (which is > alphabetical, I suppose). At this point I suspect we'll end up > committing this whole feature set together anyway, so we might as well > organize it that way. > OK. I will merge them all into one and will try to keep them in the order specified in sql_features.txt. However, for documentation, it makes more sense to keep them in logical order than the alphabetical one. What are your views on this? Thanks -- Jeevan Chalke *Principal, ManagerProduct Development* edbpostgres.com
