search

ygrek / sqlgg

SQL Guided (code) Generator
https://ygrek.org/p/sqlgg/
GNU General Public License v2.0
61 stars 20 forks source link

Add JSON_REPLACE, JSON_ARRAY, and JSON_SET functions #138

Closed johnhaley81 closed 1 month ago

johnhaley81 commented 1 month ago

I saw that https://github.com/ygrek/sqlgg/issues/86 was open for a bit and I needed these specific functions so I went ahead and attempted to add them into the codebase.

This PR adds in JSON_REPLACE, JSON_ARRAY, and JSON_SET to the parser. I wrote tests for them as well and tested them locally in my project and they all work as expected.

The only issue I ran into was that I wanted to add the variadic version of JSON_SET but that takes in key/value pairs of arguments at the end which I couldn't figure out how to implement in the code. Maybe I missed something since I'm very new to this codebase?

Please LMK if there are any changes you would like me to make or if there is something that I missed that wasn't caught by the compiler and/or tests.

tatchi commented 1 month ago

I believe @jongleb is the most appropriate person to review this

jongleb commented 1 month ago

@tatchi Thanks for ping, i didn't see it.

@johnhaley81 I had plans to add these after adding a separate json type, and I also wanted to provide the user with the opportunity to define from and to for him by passing some yojson. But in anycase, I understand that you urgently need these functions and the code looks good, so I will do it later. Text json is okay for now.

variadic version of JSON_SET

I would say to take a look how coalesce works. But I was waiting for the second time usage of the variadic to implement more general solution. So, if it's okay currently to use only 3 required arguments, let's keep it as is. depending on how urgently you need these functions and whether you need variadiс right now

johnhaley81 commented 1 month ago

So, if it's okay currently to use only 3 required arguments, let's keep it as is.

@jongleb I think having the non-variadic version of JSON_SET is good for the moment. This is good for me if it's good for you guys.