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

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

Merged
merged 16 commits into from
Jun 20, 2024

Conversation

m-span
Copy link
Collaborator

@m-span m-span commented Jun 18, 2024

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:

  • Move all files from prqlc-ast to prqlc-parser. prqlc-ast is to be deprecated
  • Within parser stage, rename objects, modules and methods to meet new spec. Organize lexer and parser stage into separate modules.

@m-span
Copy link
Collaborator Author

m-span commented Jun 18, 2024

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
Copy link
Member

max-sixty commented Jun 18, 2024

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 #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
Copy link
Member

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

@m-span m-span marked this pull request as ready for review June 20, 2024 00:00
@m-span
Copy link
Collaborator Author

m-span commented Jun 20, 2024

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
Copy link
Collaborator Author

m-span commented Jun 20, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

m-span commented Jun 20, 2024

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...

No longer needed; we're not going to have multiple copies of AST items any longer...
@max-sixty
Copy link
Member

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...)

Copy link
Member

@max-sixty max-sixty Jun 20, 2024

Choose a reason for hiding this comment

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

Same here — IIUC it would be strictly better to have only one of these — no strong view on which

@max-sixty
Copy link
Member

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 max-sixty enabled auto-merge (squash) June 20, 2024 01:52
@max-sixty
Copy link
Member

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

@m-span
Copy link
Collaborator Author

m-span commented Jun 20, 2024

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
Copy link
Collaborator Author

m-span commented Jun 20, 2024

(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?

@m-span m-span disabled auto-merge June 20, 2024 02:13
@max-sixty
Copy link
Member

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 max-sixty changed the title refactor: prqlc-parser major reorg changes, deprecate prqlc-ast refactor!: prqlc-parser major reorg changes, deprecate prqlc-ast Jun 20, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@max-sixty max-sixty enabled auto-merge (squash) June 20, 2024 02:46
@max-sixty max-sixty changed the title refactor!: prqlc-parser major reorg changes, deprecate prqlc-ast refactor!: prqlc-parser major reorg changes, remove prqlc-ast Jun 20, 2024
@max-sixty max-sixty merged commit 6357126 into PRQL:main Jun 20, 2024
85 of 86 checks passed
@max-sixty
Copy link
Member

Thank you @m-span !

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.

2 participants