Skip to content

Conversation

LucaCappelletti94
Copy link
Contributor

This PR introduces the From trait implementation for all variants of the Statement enum which are evidently mapped 1-1. There are a few variants which are not mappable in such a way, and therefore I refrained from creating one From impl for those. I have also added some .into() in the code where I have seen clear examples of statements conversion which may benefit from using the From trait instead of specifying the Statement variant. There are certainly more such cases, but I thought it was outside of the scope of this PR to replace such things.

This PR closes #2020

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this I'm thinking it could make more sense to have the impls right next to the struct definitions (similar to how we do for display impls) vs a dedicated file for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created the different module as the main one is ever more massive, so I thought it best not to add even more to it. That being said, if you prefer avoiding specialised sub modules but keep the monolitic one I can copy everything back into it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we can move the impls close to the associated struct definitions, indeed it would be nice to split the large files but I imagine splitting based on e.g. statement types or similar dimension grouping related structs in the same file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in commit 9ef033f

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @LucaCappelletti94!
cc @alamb

@iffyio iffyio added this pull request to the merge queue Sep 16, 2025
Merged via the queue into apache:main with commit 7021561 Sep 16, 2025
10 checks passed
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.

Implementing From for sqlparser::ast::Statement variants

2 participants