Hi Vik, On Thu, Mar 27, 2025 at 3:27 PM Vik Fearing <v...@postgresfriends.org> wrote:
> > I am reviewing this from a feature perspective and not from a code > perspective. On the whole, this looks good to me from a standards point > of view. > Thank you so much for reviewing! On Thu, Mar 27, 2025 at 3:27 PM Vik Fearing <v...@postgresfriends.org> wrote: > There are two things that stand out to me about this feature: > > > 1) I am not seeing the ** syntax in the standard, so does it need to be > signaled as not supported? Perhaps I am just overlooking something. > I think you’re referring to patch v11-0008, which adds the ** token in the lexer. You’re right — the ** syntax is not mentioned at all in the SQL standard. I added lexer support for ** based on earlier feedback from Mark Dilger. See his comment below: On Thu, Mar 13, 2025 at 3:02 PM Alexandra Wang <alexandra.wang....@gmail.com> wrote: > On Tue, Mar 4, 2025 at 8:05 AM Mark Dilger <mark.dil...@enterprisedb.com> > wrote: > >> >> > +SELECT (jb).a.**.x FROM test_jsonb_dot_notation; -- not supported >> +ERROR: syntax error at or near "**" >> +LINE 1: SELECT (jb).a.**.x FROM test_jsonb_dot_notation; >> >> I wonder if it would be better to have the parser handle this case and >> raise a ERRCODE_FEATURE_NOT_SUPPORTED instead? >> > > In 0008 I added a new token named "DOUBLE_ASTERISK" to the lexers to > represent "**". Hope this helps! > The ** tests are there because ** is valid in jsonpath, and can be used in functions like json_query() and jsonb_path_query(). So I think we should make it explicit to users that ** is not supported by dot access. As long as the tests make that clear, I don’t have a strong preference whether we throw a syntax error in the lexer or a “feature not supported” error in the parser. If we prefer a syntax error, I’m happy to drop patch 0008. On Thu, Mar 27, 2025 at 3:27 PM Vik Fearing <v...@postgresfriends.org> wrote: > 2) There is no support for <JSON item method>. Since this appears to be > constructing a jsonpath query, could that not be added to the syntax and > allow jsonpath to throw the error if the function doesn't exist? > You’re right — there’s currently no support for <JSON item method>. I intentionally didn’t include it in this patch because it would again expand the scope of work. Supporting <JSON item method> seems non-trivial to me, because the current design transforms expression nodes separately for each indirection. I previously tried a patch that simply converted the full chain of indirections into a JSON_QUERY() call in transformIndirection(). The problem with that approach is that in EXPLAIN output or view definitions, we’d see the rewritten JSON_QUERY() instead of the original dot notation — which feels like a leaky abstraction. To support item methods, I think we’d need to add a new kind of indirection to represent function calls. Then, we could either: a) explicitly add each item method token in gram.y, similar to the list of method keywords in jsonpath_gram.y; or b) allow a generic method-call token in gram.y, and transform it into the corresponding JsonPathItemType that jsonpath understands. Either way, I’m happy to work on it, but would prefer to discuss and implement it in a separate follow-up patch. P.S. After reading your second question, your first one makes more sense to me — I’ve already taken the easy route of not parsing item methods and was planning to leave that work for a follow-up patch. So maybe it’s more consistent to just let the syntax error for ** happen, rather than explicitly throwing a “feature not supported” error? The only difference, I think, is that we don’t want ** in simplified accessor syntax, whereas we do want to support item methods in the future. Let me know what you think! Best, Alex
