apache / datafusion-sqlparser-rs

Extensible SQL Lexer and Parser for Rust
Apache License 2.0
2.82k stars 543 forks source link

Suggest splitting `parser.rs` into smaller mod files #944

Open r4ntix opened 1 year ago

r4ntix commented 1 year ago

Background

The code of current parser.rs is too large, it's difficult to read and maintain. I'd suggest splitting it into smaller mod files.

Proposal

I have split the ALTER ROLE code into parser/alter.rs in this PR feat: add ALTER ROLE syntax of PostgreSQL and MS SQL Server by r4ntix · Pull Request #942 · sqlparser-rs/sqlparser-rs (github.com)

I think we can do a refactoring and split the parser, like this:

./src/parser/
|-- alter.rs
|-- create.rs
|-- drop.rs
|-- mod.rs
|-- select.rs
|-- ...
`-- utils.rs

I wonder what people would suggest about this?

Implementation Plan

we can break it up into some issues and PRs to step through this refactoring.

DDL

all tasks:

DML

all tasks:

DCL

all tasks:

PostgreSQL/BigQuery/MySQL/SparkSQL/ClickHouse/Hive

all tasks:

r4ntix commented 1 year ago

The overall directory structure after sorting is as follows:

./src/parser/
|-- alter.rs
|-- analyze.rs
|-- assert.rs
|-- begin.rs
|-- cache.rs
|-- close.rs
|-- commit.rs
|-- copy.rs
|-- create.rs
|-- deallocate.rs
|-- declare.rs
|-- delete.rs
|-- describe.rs
|-- discard.rs
|-- drop.rs
|-- execute.rs
|-- explain.rs
|-- fetch.rs
|-- grant.rs
|-- insert.rs
|-- kill.rs
|-- merge.rs
|-- mod.rs
|-- msck.rs
|-- prepare.rs
|-- revoke.rs
|-- rollback.rs
|-- savepoint.rs
|-- select.rs
|-- set.rs
|-- show.rs
|-- start.rs
|-- truncate.rs
|-- uncache.rs
|-- update.rs
|-- use.rs
`-- utils.rs
alamb commented 1 year ago

This basic plan seems reasonable to me, though it will cause substantial conflicts with anyone who as an outstaning PR 🤔

r4ntix commented 1 year ago

@alamb If possible, we can break it up into some issues and PRs to step through this refactoring. Do you have any suggestions for this? Thank you very much. 😊

alamb commented 1 year ago

@alamb If possible, we can break it up into some issues and PRs to step through this refactoring.

Sounds like a good plan for me

Do you have any suggestions for this? Thank you very much. 😊

My bandwidth for this repo is quite low. However, if you make PRs that are easy to review (just moving code around) I think I can review and merge them pretty fast

AugustoFKL commented 1 year ago

@alamb one question, not 100% related but... What are your thoughts on creating a trait Parse to implement for the structures/enums, instead of loose functions?

This would make it easier to identify where things happen. Sometimes I find myself trying to figure which method parses which structure, as they are all together and, sometimes, with the same return.

alamb commented 1 year ago

What are your thoughts on creating a trait Parse to implement for the structures/enums, instead of loose functions?

I am not quite sure what you are proposing @AugustoFKL -- what would be an example of such a trait?