Add derive based AST visitor #765

tustvold · 2022-12-15T19:22:45Z

Reimplements #758 but using a proc-macro.

One issue is that currently this will not dispatch to visit_table_name for statements where the ObjectType is parameterised, this is Drop, ShowCreate, Comment. We could theoretically add a skip_if option down the line, but I wanted to keep things simple for now.

alamb · 2022-12-15T19:26:21Z

Thanks @tustvold -- I will check this one out carefully, but probably not until tomorrow

alamb · 2022-12-16T14:10:33Z

Thank you -- I plan to review this carefully later today

alamb

Thank you @tustvold -- I just had a chance to go through this code in more detail 👍

I think it works well for the usecase of visiting parts

I also spent some time comparing it to the approach in #601 and I had a queston:

How hard do you think it would be to add the following features (in the future):

A Rewriter / mutator for performing SQL rewrites in addition to visiting,
A previsit() and postvisit() to specify the order in which children were visted (DFS or BFS)?
Additional "visit" methods (e.g. if I wanted to visit all Idents)? Could this be done in the proc macro itself? It seems like Drive proposed in Add an AST visitor #601 can do so.

I think 3. would be particularly compelling for other users of this crate in the near term.

cc @lovasoa -- I wonder if you have thoughts on this API

cc @AugustoFKL for your guidance

alamb · 2022-12-17T12:40:30Z

derive/Cargo.toml

+name = "sqlparser_derive"
+description = "proc macro for sqlparser"
+version = "0.1.0"
+authors = ["Andy Grove <andygrove73@gmail.com>"]


🤔 maybe this should be something different. I will think about a more appropriate author -- maybe sqlparser-rs contributors 🤔

TBH I'm not sure as well. Theoretically I'd say that the owner/creator should be the author, but since this is an expansion, not sure whether put both of you or just the @tustvold

I think it is ok as is

I changed it to "sqlparser authors" in #775

alamb · 2022-12-18T11:10:25Z

Cargo.toml

@@ -32,6 +33,7 @@ serde = { version = "1.0", features = ["derive"], optional = true }
 # of dev-dependencies because of
 # https://github.com/rust-lang/cargo/issues/1596
 serde_json = { version = "1.0", optional = true }
+sqlparser_derive = { version = "0.1", path = "derive", optional = true }


Note to myself: if we merge this PR I think we should document this feature flag -- it seems documentation is missing for the existing feature flags

~~Docs in #774~~ #776

alamb · 2022-12-18T11:14:58Z

derive/README.md

+
+## Visit
+
+This crate contains a procedural macro that can automatically derive implementations of the `Visit` trait


Note to self -- this should have a doc link back to the sqlparser crate

derive/src/lib.rs

alamb · 2022-12-18T11:18:16Z

src/ast/data_type.rs

@@ -337,6 +341,7 @@ fn format_datetime_precision_and_tz(
 /// guarantee compatibility with the input query we must maintain its exact information.
 #[derive(Debug, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit))]


I am a big fan of this method as a way to minimize maintenance burden on new contributions

I picked a random struct and removed the #[cfg_attr(feature = "visitor", derive(Visit))] to see what would happen if it was forgotten

I think the error is fairly clear 👍

Compiling sqlparser v0.28.0 (/Users/alamb/Software/sqlparser-rs) error[E0277]: the trait bound `ast::Password: visitor::Visit` is not satisfied --> src/ast/mod.rs:1262:9 | 1262 | password: Option<Password>, | ^^^^^^^^ the trait `visitor::Visit` is not implemented for `ast::Password` | = help: the following other types implement trait `visitor::Visit`: Box<T> CommentObject CreateTableBuilder Keyword Option<T> String Vec<T> ast::Action and 120 others note: required for `Option<ast::Password>` to implement `visitor::Visit` --> src/ast/visitor.rs:21:16 | 21 | impl<T: Visit> Visit for Option<T> { | ^^^^^ ^^^^^^^^^

It's also making it easier to keep up with the merge conflicts 😆

alamb · 2022-12-18T11:20:38Z

src/ast/visitor.rs

+pub trait Visitor {
+    type Break;
+
+    /// Invoked for any tables, virtual or otherwise that appear in the AST


Can we please rename this to visit_relation (which is a more general term for table / view / CTE, etc)?

alamb · 2022-12-18T11:28:46Z

.gitignore

@@ -2,6 +2,7 @@
 # will have compiled files and executables
 /target/
 /sqlparser_bench/target/
+/derive/target/


Also note to self

I tested using

cargo test --all --features=visitor

We need to add this feature to the CI tests

Oh, that explains the poor coverage I guess

I verified that the CI does run with --all-features: https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/3794845435/jobs/6453351547

https://github.com/sqlparser-rs/sqlparser-rs/blob/dec3c2b818bdedcc81f0e3990e2dd2fefcf9e872/.github/workflows/rust.yml#L67

coveralls · 2022-12-18T11:32:11Z

Pull Request Test Coverage Report for Build 3794824925

0 of 74 (0.0%) changed or added relevant lines in 3 files are covered.
305 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 85.985%

Changes Missing Coverage	Changed/Added Lines	%
src/lib.rs	1	0.0%
derive/src/lib.rs	27	0.0%
src/ast/visitor.rs	46	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	305	10.05%

Totals
Change from base Build 3794463597:	-0.4%
Covered Lines:	13007
Relevant Lines:	15127

💛 - Coveralls

tustvold · 2022-12-21T11:36:34Z

Additional "visit" methods (e.g. if I wanted to visit all Idents)? Could this be done in the proc macro itself? It seems like Drive proposed in #601 can do so.

Very easy, it would simply be a case of adding the method to the Visitor and then potentially annotating a couple of fields and/or structs with visit(with = "my_amazing_visit_method")

A previsit() and postvisit() to specify the order in which children were visted (DFS or BFS)?

Currently the code implements previsit, it would be relatively straightforward to add an additional annotation to support post visit.

A Rewriter / mutator for performing SQL rewrites in addition to visiting,

You could definitely apply the same broad approach to this problem, but without having a good idea as to what the API might look like, or what the precise use-case would be, I'm not sure how easy this would be in practice. I suspect rewriting SQL AST directly would be relatively non-trivial to do correctly, not to mention verbose, with a far better approach to first lower to some sort of logical/intermediate representation.

lovasoa · 2022-12-22T09:43:18Z

I would love this to be merged!

This re-implements the entire derive-visitor crate inside sqlparser-rs, as opposed to my approach in #601 that just added it as a dependency. But I would still be very happy if this were merged and we finally had a way to visit sql statements !

AugustoFKL

@tustvold any reasons why the coverage is so small?

alamb

I think we should do it.

I'll plan to resolve outstanding conflicts and then add additional documentation / tests as a follow on PR.

Thank you @tustvold @AugustoFKL and @lovasoa

alamb · 2022-12-28T14:44:20Z

derive/Cargo.toml

+name = "sqlparser_derive"
+description = "proc macro for sqlparser"
+version = "0.1.0"
+authors = ["Andy Grove <andygrove73@gmail.com>"]


I think it is ok as is

alamb · 2022-12-28T15:07:15Z

Thanks again all!

alamb · 2022-12-28T17:03:28Z

Improved Docs: #778

Add derive based AST visitor

1f381f4

tustvold added 3 commits December 15, 2022 19:34

Fix BigDecimal

e882183

Fix no visitor feature

0cafaf9

Add test

3f4387a

Merge remote-tracking branch 'upstream/main' into proc-macro-ast-visitor

89cb26a

tustvold mentioned this pull request Dec 16, 2022

Add manually implemented AST visitor #758

Closed

alamb reviewed Dec 18, 2022

View reviewed changes

tustvold added 4 commits December 21, 2022 12:30

Rename visit_table to visit_relation

6dc8367

Merge remote-tracking branch 'upstream/main' into proc-macro-ast-visitor

387bdbe

Review feedback

971b422

Add pre and post visit

c869d20

tustvold requested a review from alamb December 22, 2022 09:25

AugustoFKL reviewed Dec 22, 2022

View reviewed changes

alamb approved these changes Dec 28, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into proc-macro-ast-visitor

26b3289

alamb merged commit dec3c2b into apache:main Dec 28, 2022

This was referenced Dec 28, 2022

Rename sqlparser_derive to sqlparser_procmacro, document features #775

Closed

Improve docs and add examples for Visitor #778

Merged

This was referenced Dec 31, 2022

Add an AST visitor #601

Closed

Add a mutable visitor #782

Merged

xxchan mentioned this pull request Jan 8, 2024

feat: redact SqlOption in SHOW CREATE risingwavelabs/risingwave#14310

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add derive based AST visitor #765

Add derive based AST visitor #765

tustvold commented Dec 15, 2022

alamb commented Dec 15, 2022

alamb commented Dec 16, 2022

alamb left a comment

alamb Dec 17, 2022

AugustoFKL Dec 22, 2022

alamb Dec 28, 2022

alamb Dec 28, 2022

alamb Dec 18, 2022

alamb Dec 28, 2022 •

edited

Loading

alamb Dec 18, 2022

alamb Dec 28, 2022

alamb Dec 18, 2022

tustvold Dec 21, 2022

alamb Dec 18, 2022

alamb Dec 18, 2022

AugustoFKL Dec 22, 2022

alamb Dec 28, 2022

coveralls commented Dec 18, 2022 •

edited

Loading

tustvold commented Dec 21, 2022

lovasoa commented Dec 22, 2022 •

edited

Loading

AugustoFKL left a comment

alamb left a comment

alamb Dec 28, 2022

alamb commented Dec 28, 2022 •

edited

Loading

alamb commented Dec 28, 2022 •

edited

Loading


		## Visit

		This crate contains a procedural macro that can automatically derive implementations of the `Visit` trait

Add derive based AST visitor #765

Add derive based AST visitor #765

Conversation

tustvold commented Dec 15, 2022

alamb commented Dec 15, 2022

alamb commented Dec 16, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3794824925

💛 - Coveralls

tustvold commented Dec 21, 2022

lovasoa commented Dec 22, 2022 • edited Loading

AugustoFKL left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 28, 2022 • edited Loading

alamb commented Dec 28, 2022 • edited Loading

alamb Dec 28, 2022 •

edited

Loading

coveralls commented Dec 18, 2022 •

edited

Loading

lovasoa commented Dec 22, 2022 •

edited

Loading

alamb commented Dec 28, 2022 •

edited

Loading

alamb commented Dec 28, 2022 •

edited

Loading