Change prql.target extension setting default to Generic and add None option

Currently the default target setting is Ansi.

My mental model is that it should be None, since if we start turning on warnings / errors where this value is different from the value in the query header, they would almost always be triggered with the current approach

I was looking at that today too as I was checking the CompileOptions exposed by the prql-js wrapper and the underlying prql compiler implemented in Rust.

We set the default to Ansi as the first and most likely more standard SQL target option.

Do we mean this by Ansi SQL? https://blog.ansi.org/2018/10/sql-standard-iso-iec-9075-2016-ansi-x3-135/

In any case, when using PRQL with different database systems, expectations are you'd set the target to what flavor of SQL to output.

It might be confusing to have None. Does that mean no SQL output is generated?

The other option we have is Generic. Also, not clear what that represents. Is it the same as Ansi?

@RandomFractals

It might be confusing to have None. Does that mean no SQL output is generated?

Yes, good point.

Ideally, we want an option which is "defer to the query"; so far that's been None. Currently, when None is passed to the function, and there's no text in the query header, we default to generic SQL. In the future, I guess that might be presumptuous if we were to we compile to more than SQL.

Ref PRQL/prql#1686.

The other option we have is Generic. Also, not clear what that represents. Is it the same as Ansi?

They are functionally the same at the moment; I definitely agree Generic is better than Ansi, since it's closer to None.

The issue will come if we turn on warnings / errors on the values being different — they'd get triggered anytime someone supplied target:sql.postgres, because the compiler would see Generic was passed.

Some options:

Default to Generic, resolve how we handle disagreements between the function arg and the query header later (e.g. we could not warn on Generic, but then there's a special-case)
Default to None, resolve whether that's presumptuous in the distant future

WDYT @RandomFractals ?

@max-sixty

@max-sixty & @aljazerzen I propose we do the following to provide an option that lets users overwrite prql target in .prql file with None:

Add None to prql.target enum in extension settings config in package.json
Document what that option means in new prql settings md table we'll add in Document new PRQL Settings under Configuration section in README.md #97
Provide a link to prql target:sql.<dialect> documentation page from PRQL Book that contains examples of setting sql target and system dialect in prql file header
Skip setting and passing target option in CompileOptions when ext. prql.target setting is None
Default prql.target setting to Generic as that is the current prql compiler default too.

These changes would allow users to specify and use prql target header in .prql files and ignore extension prql.target setting in such cases.

I would suggest the ambiguity and difference between Ansi and Generic SQL flavor targets is documented in PRQL book target documentation page. Most users would probably know, can guess, or search to learn more about what Ansi SQL means, and might be thrown off by the Generic option. Why I set it to Ansi default in the initial version of this new extension prql.target setting feature implementation.

My understanding is that this has to do with prql-js default values.
Currently the default is "sql.generic".
https://github.com/PRQL/prql/blob/7c42e5c64b92e9e43b0be82000135663552f0972/prql-js/src/lib.rs#L106

I noticed this in PRQL/prql#1750, but left the default as Generic to let it pass existing test cases.
PRQL/prql@0ba1b23

BTW, in prqlr, it is None.
https://github.com/eitsupi/prqlr/blob/a4ceb55bb02bbf7813482b6ce764dfc3aed12f38/src/rust/src/lib.rs#L21

@eitsupi

@eitsupi that's good to know. I still think either Generic or Ansi as a more standard option default would make more sense.

We'll add None option so that users can specify their target in prql file header. However, I don't think we should be passing that target option to our compile call with prql-js, and the compiler should just produce "generic" sql or check prql target header in the file in such scenario, i.e. when target is not defined in CompileOptions we pass.

I would even argue that if we default to Generic and there is prql target:sql.sqlite set in the prql query file, PRQL compiler should generate SQLite flavor of that query, i.e. same as what None currently does per your definition of it in the core lib.

Only None falls back to the header.
if there is nothing in the header after falling back from None to the header, it falls back to Gneric.

https://github.com/PRQL/prql/blob/7c42e5c64b92e9e43b0be82000135663552f0972/prql-compiler/src/sql/gen_query.rs#L26-L36
https://github.com/PRQL/prql/blob/7c42e5c64b92e9e43b0be82000135663552f0972/prql-compiler/src/sql/dialect.rs#L89-L93

So basically the default should be None in bindings.

I created a issue about that PRQL/prql#1855

@eitsupi

@eitsupi well, looking at prql-js tests, that None option can be any value not in the supported sql.* dialects list?

I found opts.target = "sql.not_existing"; in https://github.com/PRQL/prql/blob/main/prql-js/tests/test_all.js#L75

Should it be sql.none or none instead?

Please document it in prql-js lib docs: https://github.com/PRQL/prql/blob/main/prql-js/README.md

@RandomFractals

@RandomFractals I think the only way to set it to None from the JS side at the moment is by not passing anything to the CompileOptions.
https://github.com/PRQL/prql/blob/7c42e5c64b92e9e43b0be82000135663552f0972/prql-js/src/lib.rs#L104-L114

If nothing is passed, Target will be Sql(None).
https://github.com/PRQL/prql/blob/7c42e5c64b92e9e43b0be82000135663552f0972/prql-compiler/src/lib.rs#L138-L142

So, we need to change the default value of prql-js, I create a PR.

ok. this seems to work. I'd rather set default to Generic and will pass sql.none for the None option which is ignored by the prql-js compiler v0.5.0 and produces "generic" sql too, if no prql target:sql.* dialiect setting is found in the .prql file header. See: 9e9626a

tbh, I wish those options were better documented in prql-js docs for the users of that JS library.

Not sure why our compile.ts can't find it. I do see it published and npm install update worked just fine after bumping it in package.json ...

@max-sixty

@max-sixty this is what I see in /node_modules after update to prql-js v0.5.1:

i.e. no js in that published module. Might need another repackage and publish to npmjs.

Should be more like what we get with prql-js v0.5.0:

We can confirm it in the logs.

0.5.0
https://github.com/PRQL/prql/actions/runs/4130439718/jobs/7137127192#step:4:1
0.5.1
https://github.com/PRQL/prql/actions/runs/4207097290/jobs/7301452313#step:4:1

I'm not sure what happened with 0.5.1 because the build steps look the same......

sounds like prql-js bundling has been fixed in PRQL/prql#1887

@max-sixty

@max-sixty still not sure if prql-js v0.5.2 actually generates sql for the requested target in prql file header.

I am going to close this vscode extension ticket as default to Generic target update is done and I added None option.

@eitsupi & @aljazerzen might need to push another prql-js CompileOptions.target fix for the rest to work.

Try

prql target:sql.mssql

...

... and check if take is compiled to TOP.

(target is not emitted in the signature if CompileOptions.target is not set)

yep. that works. So, I guess None option is good to go now too.

I'll doc the rest and will publish new release tomorrow.

Navigation Menu

Repository navigation

Change prql.target extension setting default to Generic and add None option #98

22 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Change prql.target extension setting default to Generic and add None option #98

Description

Activity

RandomFractals commented on Feb 15, 2023

max-sixty commented on Feb 16, 2023

RandomFractals commented on Feb 16, 2023

eitsupi commented on Feb 16, 2023

RandomFractals commented on Feb 16, 2023

RandomFractals commented on Feb 16, 2023

eitsupi commented on Feb 16, 2023

eitsupi commented on Feb 16, 2023

RandomFractals commented on Feb 16, 2023

eitsupi commented on Feb 16, 2023

RandomFractals commented on Feb 16, 2023

22 remaining items

RandomFractals commented on Feb 18, 2023

RandomFractals commented on Feb 18, 2023

eitsupi commented on Feb 18, 2023

RandomFractals commented on Feb 19, 2023

RandomFractals commented on Feb 19, 2023

aljazerzen commented on Feb 19, 2023

RandomFractals commented on Feb 19, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions