Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow multiple queries per file? #1825

Closed
max-sixty opened this issue Feb 13, 2023 · 10 comments
Closed

Allow multiple queries per file? #1825

max-sixty opened this issue Feb 13, 2023 · 10 comments
Labels
language-design Changes to PRQL-the-language major-feature needs-discussion Undecided dilemma

Comments

@max-sixty
Copy link
Member

Currently we have a nice, simple structure — we can define a pipeline consisting of transformations, and optionally including some parent pipelines with let.

One quite nice thing about Malloy is the ability to export multiple pipelines in a file (example). They can then be shared across files.

One starting idea for how the this could work in PRQL

  • If there's a "main" pipeline at the end, then we compile that
  • But if there isn't, there are only variables defined with let, it becomes a "module" file; and we compile all the relation variables (aka CTEs) and return a map of them
    • Maybe with some other function than compile, like import
  • We could define how to do actual imports separately

It's important the project remains focused — it's very easy to try to do everything and do nothing well — we still have lots to do on the basic pipeline. So possibly this is something that should come later.

@max-sixty max-sixty added language-design Changes to PRQL-the-language major-feature labels Feb 13, 2023
@aljazerzen
Copy link
Member

I agree, this would be useful and proposed idea seems sound.

@max-sixty
Copy link
Member Author

Related to #1803

@max-sixty max-sixty added the needs-discussion Undecided dilemma label Feb 24, 2023
@max-sixty
Copy link
Member Author

I'm trying to think of a good API for the compile function when passed a file with a main pipeline:

  1. It returns the CTEs for tables, and nothing for any functions
    • When we properly support other types with let, what should happen then? We put those in CTEs too?
  2. It returns some JSON-like representation of the various items
    • Maybe it requires a target=sql.generic.json flag??

I'd be up for doing this fairly soon if we can get to consensus on this.

@aljazerzen
Copy link
Member

I don't think returning just CTEs is a good idea - it not valid SQL and is not easily composable - it there a WITH in the front? Can there be a WITH RECURSIVE? Is there a following comma?

I'd rather suggest that:

  • compile throws an error if there is no main pipeline,
  • pq will fail with an error if there is no main pipeline which is what we want,
  • we can add ErrorMessage::code, so vscode extension can just ignore the error and display a small "no main pipeline" in the preview,

For importing other files, I'd suggest we have a different interface, which would have a SourceLoader provided by the client and used by compiler when an external file is referenced.

@max-sixty
Copy link
Member Author

max-sixty commented Feb 25, 2023

Very much agree @aljazerzen; great design


we can add ErrorMessage::code, so vscode extension can just ignore the error and display a small "no main pipeline" in the preview,

So we'd have two fields — a string message and a (string?) code / type?

In python we could produce custom errors — different types (though not an Enum). I'd have to look at how that would work with the bindings framework; do we produce a "a string message and a (string?) code / type" and then reconstitute in python...

If we're not sure, we could also make this change at the VSCode extension level for the moment and just manually parse the error.


display a small "no main pipeline" in the preview,

My nit is that this follows me around suggesting I have errors in the project; unrelated to the preview.

image

@aljazerzen
Copy link
Member

So we'd have two fields — a string message and a (string?) code / type?

We already have quite a few, adding a string code would not be a big deal.

https://github.com/PRQL/prql/blob/main/prql-compiler/src/error.rs#L78-L90


suggesting I have errors in the project

Yeah this bothers me too.

@max-sixty
Copy link
Member Author

So we'd have two fields — a string message and a (string?) code / type?

We already have quite a few, adding a string code would not be a big deal.

https://github.com/PRQL/prql/blob/main/prql-compiler/src/error.rs#L78-L90

Ah, I see, we actually serialize this whole struct into a JSON string and send it to JS:

Err(e) => wasm_bindgen::throw_str(&e.to_json()),
.

How about we start by including an error_type Enum as a member on that struct, default to something generic, and allow different values. It'll appear to JS as a string in the JSON.

@max-sixty
Copy link
Member Author

Actually, while the error types sounds like a good proposal, I don't think it's the closest way of solving the "missing main pipeline". I'm going to comment on #1803, and we can dedicate this issue to just allowing multiple queries per file

@aljazerzen
Copy link
Member

My bad, I mixed up the issues here. Yes, let's keep this issue for modules, exporting definitions out of files, I have a few unfinished thoughts about this.

@max-sixty
Copy link
Member Author

Closed by #2129 (not yet complete, but that contains the path forward)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
language-design Changes to PRQL-the-language major-feature needs-discussion Undecided dilemma
Projects
None yet
Development

No branches or pull requests

2 participants