Repository navigation
target precedence #1686
Description
From PRQL/prql-vscode#48 (comment)
If someone passes a value of target to the compile function, and the query specifies a value, which should take precedence?
Currently the value passed to compile. But generally it should be the more "local" value, which is probably the value in the query.
Re the "local" principle — for example, if you have a global git setting and a setting for a specific repo, the setting in the specific repo takes precedence. Though this isn't always the case; e.g. for authorization APIs...
Activity
eitsupi commentedon Feb 6, 2023
As I mentioned in PRQL/prql-vscode#48 (comment), I don't find the current behavior odd, but I do think that the user may not get the desired output regardless of which is preferred.
Perhaps compiler could mention the target in the comments of the output SQL?
max-sixty commentedon Feb 6, 2023
Good idea! I'll make another issue, would be a good first issue
aljazerzen commentedon Feb 7, 2023
My line of though when implementing this behavior was to give precedence to the caller of the compile function, as they know what this file will be used for. It is explicitly noted in the documentation that passing None for dialect will fallback to the
prql target:...header, which is intended to be used when the SQL query will not be executed immediately and we want to display it instead.Imagine writing a PRQL query engine that heeds
prql target:...over the dialect of RDBMS which will be used to execute the query...So I vote to keep current behavior.
snth commentedon Feb 7, 2023
I agree with @aljazerzen .
Another scenario (not too importand at the moment), which IMHO supports this, is to consider what would happen with multiple files queries. We're not there yet but the aim is to allow importing functions and pipelines from other files. What should be heeded when there are multiple conflicting targets specified in different files? I guess you could the one of the final file/sink that everything is imported into but better to go with the option passed to the compile function as suggested.
I was going to ask whether it's worth keeping the
target:...in the header at all, but @aljazerzen made the point that itThat makes sense and I support that.
max-sixty commentedon Feb 8, 2023
On reflection, +1 for giving the engine precedence — it has more information than the query.
I still think allowing the query to specify the target is useful — particularly when first trying PRQL.
We don't have a system of warnings, and raising an error may be too severe (or not?). When we do get warnings & lints, we can issue one if there's a disagreement between the two.
Sounds fairly unanimous, so I'll close. Feel free to comment if we think it should raise an error.