Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds ability to deparse AST back into a SQL string.
We might not need this PR
After starting work on this, I realized there was this PR that already implements this. But since I took a different approach which may reduce bundle size, I thought I'd still open a PR. Might be worth picking some pieces from this PR if there's value.
Background
Under the hood
libpg_query
supports both parsing and deparsing. It supports parsing SQL into JSON or protobuf, but it only supports deparsing from protobuf, not JSON.libpg-query-node
uses the JSON approach for parsing, so this makes deparsing tricky. We need a way to first convert the AST to a protobuf, then pass intolibpg_query
's deparse function.What this approach does differently
This approach does the JSON -> protobuf conversion in C++ vs JS. It uses the protobuf C++ lib to convert the JSON AST to a profobuf directly in C++ before passing to deparse.
libpg_query
actually comes with a protobuf generated C++ file, so no pre-processing step was required to compile the.proto
file to another language.Affect on output size
queryparser.node
went from ~2.8Mb to~5.9Mb7.7Mb (static linking, macOS). This is more than double the size which is not ideal, but maybe still smaller than using a protobuf generated JS file. For my own use, the bigger concern would be the WASM file size increase, which I haven't got building yet (see below).If we want the ability to deparse though, I'm not sure there is any way around bringing in a big protobuf generated file, apart from manually implementing a JSON deparsing function and upstreaming to
libpg_query
(probably a lot of work?).Recommendation
Offer 2 variants - one with deparse included and one without so that those who don't care about deparsing can benefit from a smaller bundle size. This probably only applies to WASM. Something like:
Any downsides?
The biggest challenge is the libprotobuf C++ dependency. People will need this lib available on their machines in order for these bindings to compile. We could clone the protobuf repo and build it ourselves via a script (like we do with
libpg_query
), but this is probably more complicated since it uses build tools like bazel. We could try to rely on prebuilt versions of libprotobuf targeting different platforms, but I haven't found a pre-built emscripten version of protobuf yet, so that's a blocker for WASM support.