PRQL / prql

PRQL is a modern language for transforming data — a simple, powerful, pipelined SQL replacement
https://prql-lang.org
Apache License 2.0
9.58k stars 208 forks source link

refactor!: prqlc-parser major reorg changes, remove prqlc-ast #4634

Closed m-span closed 1 week ago

m-span commented 2 weeks ago

From #4603 , we have the following new terminology:

stage sub-stage AST
parse lexer string -> LR — Lexer Representation
parse parser tokens -> PR — Parser Representation
semantic ast_expand PR -> PL — Pipelined Language
semantic resolver PL
semantic flatten PL
semantic lowering PL -> RQ — Resolved Query
sql preprocess RQ
sql srq-compiler RQ -> PQ — Partitioned Query
sql postprocess PQ
sql sql-compiler PQ -> sqlparser::ast
sql codegen sqlparser::ast -> string

Goals:

m-span commented 2 weeks ago

still a wip, need to make sure all references are updated correctly, but feel free to take a look and comment at how prqlc-parser is now organized

max-sixty commented 2 weeks ago

Looking great at first glance!

(Unfortunately it looks like it may not be based off the latest main and so have some conflicts; worth pulling the latest main... really hope that's not a painful merge. To the extent it's caused by https://github.com/PRQL/prql/pull/4522, there are instructions there for how to reproduce that; so running the same commands on this branch should produce a very similar, easily-mergable result)

max-sixty commented 2 weeks ago

Super minor but possibly standards are for pr to be lowercase; no view from me beyond following the rust naming standards

m-span commented 1 week ago

There's more work to be done, but this is a good first step. We might need to talk about how we want to be re-exporting our imports. These changes are absolutely Major version number update-API-breaking changes, since we are re-naming our objects.

m-span commented 1 week ago

I tried to do the least amount of changes that accomplishes the goal, but its still a lot. Requesting many eyes on this. Also, some test are suddenly failing - trying to figure it out, but if anyone has any solutions, feel free to suggest or push.

max-sixty commented 1 week ago

I tried to do the least amount of changes that accomplishes the goal, but its still a lot. Requesting many eyes on this. Also, some test are suddenly failing - trying to figure it out, but if anyone has any solutions, feel free to suggest or push.

Yes perfect. I have a few small things to merge, and possibly a fairly big thing in #4639, so great if we can do these in smallish PRs with smaller conflicts.

A couple test failures indeed look surprising. Sometimes I realize I'm importing the wrong Expr etc; so I guess that's possible here?!

(a couple are easy clippy ones, will probably auto-fix with clippy --fix)

m-span commented 1 week ago

there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...

max-sixty commented 1 week ago

there seems to be issues with the ast_code_matches.rs tests. What is the purpose of these tests? They don't seem to tolerate refactors well...

Funny you asked, I actually just pushed a change removing them. (They existed as a compromise to having multiple copies of similar AST structs around...)

max-sixty commented 1 week ago

I'm sure we'll find some more refinements but looking really good for the moment — will hit the big button.

I made some small-ish refinements, feel free to object to anything if I went against your intention...

max-sixty commented 1 week ago

(could definitely do with a changelog entry, doesn't need to be part of this PR tho)

m-span commented 1 week ago

Looking good from this side. Cleanups were great. Just a small shame I coudln't get the refactor on prqlc_parser::parse_source() working that I had to undo it via this commit. It did some nice things like hide stuff that didn't need to be exposed

m-span commented 1 week ago

(could definitely do with a changelog entry, doesn't need to be part of this PR tho)

I'll add it. Also, this is a breaking api change...should we increment the working version number to a major increment?

max-sixty commented 1 week ago

Also, this is a breaking api change...should we increment the working version number to a major increment?

Let's add ! in the PR description and then we'll do it at release time

(your approach is reasonable but it may not generalize well to multiple breaking changes in a release without another mechanism...)

max-sixty commented 1 week ago

Thank you @m-span !