Add prql.target setting and use it to compile PRQL to SQL

This would allow users to configure SQL dialect to use per project via VSCode workspace settings and instruct PRQL extension to produce SQL for different data sources.

Current list of SQL supported dialects from: https://github.com/PRQL/prql/blob/main/prql-js/src/lib.rs#L124

pub enum Dialect {
    Ansi,
    BigQuery,
    ClickHouse,
    Generic,
    Hive,
    MsSql,
    MySql,
    PostgreSql,
    SQLite,
    Snowflake,
    DuckDb,
}

This is a good idea, +1 from me.

A note about the behavior: compiler will use the dialect passed in SQLCompileOptions and fallback to target specified in prql target:... header. This means that when the option you've proposed would be set in for a workspace, all queries would use it, even if they specify the target in their header.

+1 from me too!

A note about the behavior: compiler will use the dialect passed in SQLCompileOptions and fallback to target specified in prql target:... header.

Good point.

More of a compiler question, but I wonder whether this is the right way around — generally we should have the more "local" config take precedence, and arguably the config in the query header is more local...

@RandomFractals

I don't think we need to gate this, it would be fine to take a dialect and then later change it, but ref the question from @RandomFractals on Discord:

Probably this should be called target and take sql.duckdb, given that's the external interface.

But that brings up the question of why the compiler interface has the sql::Options struct, which takes a Dialect, rather than something that takes a Target.

Have we overcomplicated the number of levels of options? Did I confuse things by changing this to target?

Oh, I was not aware of prql target option. I simply looked at prql-js api and suggested we add this prql vscode ext. setting to generate sql for the desired sql dialect. Currently, we are not doing it in compiler.ts using prql-js api: https://github.com/prql/prql-vscode/blob/main/src/compiler.ts#L5

As for the target vs dialect, I like dialect more, or we can call it prql.sqlDialectTarget if you'd like. Just target is too generic.

More of a compiler question, but I wonder whether this is the right way around — generally we should have the more "local" config take precedence, and arguably the config in the query header is more local...

(This is off-topic and we need to move the repository if we want to continue the discussion, but let me comment on one thing.)

I noticed this behavior while working on prqlr. https://github.com/eitsupi/prqlr/blob/a76602e9809f9ffb6a04de87e359818e7680291c/R/compile.R#L3

Certainly if the user is free to rewrite the query, the query is more local, but if the query is written to a file and we want to compile it to multiple targets, it would be convenient if the compiler side settings take precedence.
So I see an advantage to the current design.

@max-sixty

We discussed this issue on the call today and came to following conclusion:

we should stick to naming compile target and use dialect as sub type of SQL target,
add option prql.target which takes i.e. sql.duckdb (as @max-sixty proposed above),
rename the panel that contains SQL to "PRQL preview" (ref Rename Sql Output panel to Sql Preview #47).

This was decided based on tendency to be consistent across all our our libraries and integrations, and to use similar conventions as other languages.

@aljazerzen

@aljazerzen Sql Preview panel title, command, and view name change has been merged already. I am not following why change it back to PRQL Preview which I find confusing while looking at the open prql document on the left.

I'll use prql.target for the new compile to target setting and will implement the rest as described at the end of #42.

@aljazerzen

@aljazerzen Sql Preview panel title, command, and view name change has been merged already. I am not following why change it back to PRQL Preview which I find confusing while looking at the open prql document on the left.

You should make the final decision as the person doing it! The path of logic we discussed was that the preview of a Markdown document was called "Markdown preview", rather than "HTML preview", and so possibly it makes sense to call it "PRQL Preview".

That said, my initial reaction was more skeptical, and I / we trust your judgement...

Let's keep changes made in #47 as is for now, and open a separate ticket if we want to rework those changes.

The content of that Preview panel is SQL or some flavor of SQL generated by the PRQL compiler, so SQL Preview is more descriptive of what's shown there now.

This ticket is for the new prql.target setting. I'll do my best making those changes and we can review and adjust in the next PR before merging into main to finalize it.

@max-sixty

@max-sixty & @aljazerzen I was looking for details of the updated prql-js api with the target option.

What's the new compile(query, options) JS API signature and what properties do we need to set in options?

Do we simply use compile(query: string, { target: 'sql' })?

Also, I would need a list of string enums for the Dialect enum I found in the core repo.

We'll add prql.target setting to config in package.json as:

...
  "configuration": {
      "type": "object",
      "title": "PRQL",
      "properties": {
        "prql.target": {
          "type": "string",
          "enum": [
            "sql.duckdb",
            "sql.sqlite",
// ???
...
          ],
          "default": "sql", // or "none", what should go here?
          "description": "PRQL Compiler SQL target dialect to use by default when generating SQL from PRQL config."
        },
...

Maybe there should be CompileOptions section in this doc to list supported sql dialects, etc: https://github.com/PRQL/prql/blob/main/prql-js/README.md

Looking at js api tests: https://github.com/PRQL/prql/blob/main/prql-js/tests/test_all.js

describe("CompileOptions", () => {
    it("should be able to create from default_compile_options", () => {
      const opts = prql.default_compile_options();
      expect(() => {
        opts.dialect = prql.Dialect.Sqlite;
        assert.equal(opts.dialect, prql.Dialect.Sqlite);
      });
    });

    it("should be able to create from constructor", () => {
      const opts = new prql.CompileOptions();
      expect(() => {
        opts.dialect = prql.Dialect.Sqlite;
        assert.equal(opts.dialect, prql.Dialect.Sqlite);
      });
    });
  });

Should I set up string enum on my end and convert to your Dielact type when ext. config setting is loaded?

I can do that, but I would rather drop sql. prefix you suggested above, and just list duckdb, sqlite, etc. and default to ansi for the generic sql output.

Also, I would need a list of string enums for the Dialect enum I found in the core repo.

Do you think this can be implemented in prql-js after PRQL/prql#1727 is merged?

yeah, I can wait till PRQL/prql#1727 is merged.

To make JS api easier to use, I do think target in compile options should be an acceptable dialect name string not an enum value like I've seen in the JS tests above.

Will that PR and the other changes in the prql repo make string target value supported in options?

@eitsupi

ok. @eitsupi this is perfect! PRQL/prql@764d8d0

Thank you! I'll go ahead and map the rest in my feature branch for the ext.

@max-sixty

@max-sixty & @aljazerzen remaining: update prql-js to v0.4.3 when you release it and scrape that options mapping function.

New Prql Target Setting in the works with genered SQL and compiler version and target display in SQL Preview:

Navigation Menu

Repository navigation

Add prql.target setting and use it to compile PRQL to SQL #48

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add prql.target setting and use it to compile PRQL to SQL #48

Description

Activity

aljazerzen commented on Feb 4, 2023

max-sixty commented on Feb 4, 2023

max-sixty commented on Feb 5, 2023

RandomFractals commented on Feb 5, 2023

eitsupi commented on Feb 5, 2023

aljazerzen commented on Feb 5, 2023

RandomFractals commented on Feb 5, 2023

max-sixty commented on Feb 5, 2023

RandomFractals commented on Feb 5, 2023

RandomFractals commented on Feb 8, 2023

eitsupi commented on Feb 8, 2023

RandomFractals commented on Feb 8, 2023

RandomFractals commented on Feb 8, 2023

RandomFractals commented on Feb 8, 2023

RandomFractals commented on Feb 9, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions