prql icon indicating copy to clipboard operation
prql copied to clipboard

PRQL code generation

Open aljazerzen opened this issue 2 years ago • 1 comments

This is an onboarding issue, see instructions on how to get started.

For debugging purposes and for the prql-compiler fmt command, we have implemented a basic PRQL code generation from PL AST. It is done via impl std::fmt::Display for X, which is convenient, but lacks a few key features:

  • indentation can only be done ad-hoc, when outputting nested pipelines or lists,
  • we have no control over maximum line width,
  • we have no control on tab size.

Instead, I suggest we follow implementation of rustfmt. I've started work on codegen branch.

  • start by implementing WriteSource for pl::StmtKind. You should be able to reuse majority of impl Display for StmtKind and adapt it to use string concatenation, similar to existing code in src/codegen.rs. For now, you can fallback to using Display (via .to_string()) for FuncDef.
  • uncomment r += &stmt.kind.write(opt.clone()).unwrap(); in codegen.rs and cargo run fmt a_test_file.prql should be working

This is the gist of it. Similarly, we have adapt basically all Display impls in ast::pl to WriteSource. I suggest starting with ExprKind, which is already partially done and can easily be tested.

Because this is quite a lot of work, we can merge smaller commits implementing any small chunk of the codegen.

This issue is a prerequisite for #130.

aljazerzen avatar Jan 03 '23 16:01 aljazerzen

@Rishav1707 this is the followup on #1418

aljazerzen avatar Jan 03 '23 18:01 aljazerzen

FYI — in #2016 we test that the examples, after being formatted with the existing formatter, can still compile.

Where they can't, the language in the markdown in the book is prql_no_fmt rather than prql.

max-sixty avatar Mar 06 '23 04:03 max-sixty

FYI I deleted a couple of messages to keep the issue clean, I confused things.


@aljazerzen I think for a Good "Actually First" Issue, I think this needs an initial effort for the framework. I feel like it's at my level at the moment, but not trivially so, and so for a completely new person it might be quite confusing.

  • (Not saying you should necessarily do it, tbc. Instead just raising the issue — I've updated my view on the amount of context that a good "actually first" issue can require, and I think it should ideally be limited to a couple of files, if possible with a similar PR that's already been merged. So this could work really well after the first one)

max-sixty avatar Mar 23 '23 19:03 max-sixty

I did the framework on codegen branch, as noted in the description.

It may need to be rebased to latest main branch. I didn't t merge it because we'd have to maintain it along with existing codegen impl.

aljazerzen avatar Mar 23 '23 19:03 aljazerzen

(Thanks a lot. I added some thoughts to the broader point in #1840. Hope this isn't interpreted as criticism, am still trying to work out why we haven't been so successful here)

max-sixty avatar Mar 23 '23 19:03 max-sixty