Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add recursive iter() on ParseResult and NodeRef #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

absporl
Copy link

@absporl absporl commented Jul 24, 2024

This allows walking over all nodes in the AST, instead of just a limited subset as in the current nodes() function. See #31

The implementation uses static code generation in build.rs. The protobuf definitions are parsed, and a graph of all Message types is constructed. All NodeRef types are given an unpack() function, that recursively calls unpack() on all relevant fields (i.e., the fields that have a Node type, or that have a type that eventually has a Node type as a field).

The result is guaranteed to visit all nodes. The code generation mechanism is maybe also useful to replace parts of the codebase that currently need to be manually hardcoded.

Adds prost, prost-types and heck to the build dependencies, and updates the prost dependency version.

This allows walking over all nodes in the AST, instead of just a limited
subset as in the current `nodes()` function. See pganalyze#31

The implementation uses static code generation in `build.rs`. The
protobuf definitions are parsed, and a graph of all Message types is
constructed. All NodeRef types are given an `unpack()` function, that
recursively calls `unpack()` on all relevant fields (i.e., the fields
that have a Node type, or that have a type that eventually has a Node
type as a field).

The result is guaranteed to visit all nodes. The code generation
mechanism is maybe also useful to replace parts of the codebase that
currently need to be manually hardcoded.

Adds prost, prost-types and heck to the build dependencies, and updates
the prost dependency version.
@absporl absporl changed the title Add iter() on ParseResult and NodeRef Add recursive iter() on ParseResult and NodeRef Jul 24, 2024
@psteinroe
Copy link

psteinroe commented Aug 2, 2024

hey @absporl, I was just going to start working on the same. I am wondering why did you choose this implementation?

I am using proc macros with great success (eg here) and wanted to upstream the same to pg_query. Do you see any benefits of the one or the other?

My idea was to essentially generate most of the crate via proc macros, including enum variants etc.

@absporl
Copy link
Author

absporl commented Aug 8, 2024

Hi @psteinroe , I like your approach! It looks a lot nicer and cleaner than what I wrote. I'm not super familiar with Rust macros and only briefly considered them. My reason for the current approach is that it's close to what prost itself does, and I wanted to ensure that it would not affect compilation time of the users of this crate. But I guess the latter might also be true for macros (as in are they only evaluated when you compile the crate itself, and not when you import a crate that uses them?)?

I did have an earlier attempt where I tried to use prost-reflect to get inspectable protobuf messages at runtime, and use those, but that turned out to be quite complicated, and I also did not want runtime performance costs really.

@seanlinsley
Copy link
Member

Thanks for the PR @absporl, and your efforts as well @psteinroe. It would be interesting to compare the runtime performance of each approach (I'm less concerned about compile times), especially a version that preserves something like the Context logic to build the result for functions like ParseResult::dml_tables.

Also @psteinroe it looks like the link to your version no longer works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants