You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
display_interpolation destructured InterpolateItem::Expr { expr, .. }, discarding the format field the parser fills in from a {expr:format} suffix — so fmt rewrote s"MIN({column:0})" as s"MIN({column})". That suffix is not decoration: for s-strings it's the required binding strength of the interpolated operand, which the SQL backend reads in sql/operators.rs to decide how to parenthesize the argument. Dropping it changes generated SQL rather than just the source text.
The fix writes the suffix back out when it's present. The parser reads a format as "everything up to the closing brace" (interpolation.rs), so a format containing braces round-trips verbatim. The escapes the lexer already removed do have to be re-applied though — a " written back raw terminates the string, and a \ is swallowed on the next parse — so the suffix is written with the same \/" escaping as the literal parts.
Verified against prqlc/prqlc/src/sql/std.sql.prql, which carries 144 :0 binding strengths: before the fix prqlc fmt stripped all 144, after it preserves all 144.
let min = func column -> s"MIN({column:0})" became let min = func column -> s"MIN({column})", which resolves the operand at the enclosing function's default binding strength instead of 0.
After the fix, both counts are 144, and the affected lines survive intact:
let min = func column -> s"MIN({column:0})"
let log = func base column -> s"LOG10({column:0}) / LOG10({base:0})"
let to_text = func format column -> s"FORMAT_TIMESTAMP({format:0}, CAST({column:0} AS TIMESTAMP))"
The fmt idempotency assertion added in #6199 can't catch this: dropping the suffix is stable across a second pass, and no integration query uses a format spec.
Regression test test_interpolation_format_is_preserved covers the s-string binding-strength form and the f-string {a:>10} form; it fails on main and passes here.
cargo test -p prqlc -p prqlc-parser is green, with no snapshot churn. task prqlc:pull-request couldn't run in the sandbox — cargo-insta isn't on the path there, which is what #6144 addresses — so CI is the first full-matrix run.
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own PR, so this is a COMMENT rather than an approval.
The change itself is right, and the binding-strength reasoning holds up — translate_operator_expr in sql/operators.rs parses the suffix into required_strength and falls back to the parent strength when it's absent, so dropping it really did change generated SQL rather than just source text.
One defect in the new code: the format is written back verbatim, but the lexer strips string escapes before the interpolation parser ever sees the text, so a " or a \ inside a format is re-emitted raw. The comment's claim that the suffix "needs no escaping" is only true of braces.
in: derive x = f"{a:\"q\"}"
fmt: derive x = f"{a:"q"}" -> second pass: expected something else or '}', but found end of input
in: derive x = f"{a:\\}"
fmt: derive x = f"{a:\}" -> second pass: f"{a:}", format silently emptied
The first case means fmt can emit source that no longer parses; the second breaks the idempotency property #6199 added, for the same class of input the rest of this PR is about. Braces are the genuine exception — the parser reads the format as none_of('}') with no unescaping, so f"{a:{}" round-trips as-is and must not be doubled.
Pushing the one-line fix and test cases for all three shapes, since this is a bot PR with no author to act on the suggestion. cargo test -p prqlc -p prqlc-parser stays green with no snapshot churn.
The lexer strips string escapes before the interpolation parser runs, so writing the format back out verbatim re-emitted a raw `"` or `\` — output that either no longer parses or loses the escape on the next pass. Braces stay unescaped: the parser reads the format up to the closing brace without unescaping, so `f"{a:{}"` round-trips as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
display_interpolationdestructuredInterpolateItem::Expr { expr, .. }, discarding theformatfield the parser fills in from a{expr:format}suffix — sofmtrewrotes"MIN({column:0})"ass"MIN({column})". That suffix is not decoration: for s-strings it's the required binding strength of the interpolated operand, which the SQL backend reads insql/operators.rsto decide how to parenthesize the argument. Dropping it changes generated SQL rather than just the source text.The fix writes the suffix back out when it's present. The parser reads a format as "everything up to the closing brace" (
interpolation.rs), so a format containing braces round-trips verbatim. The escapes the lexer already removed do have to be re-applied though — a"written back raw terminates the string, and a\is swallowed on the next parse — so the suffix is written with the same\/"escaping as the literal parts.Verified against
prqlc/prqlc/src/sql/std.sql.prql, which carries 144:0binding strengths: before the fixprqlc fmtstripped all 144, after it preserves all 144.Reproduction and verification
Before, on a copy of the dialect stdlib:
let min = func column -> s"MIN({column:0})"becamelet min = func column -> s"MIN({column})", which resolves the operand at the enclosing function's default binding strength instead of 0.After the fix, both counts are 144, and the affected lines survive intact:
The
fmtidempotency assertion added in #6199 can't catch this: dropping the suffix is stable across a second pass, and no integration query uses a format spec.Regression test
test_interpolation_format_is_preservedcovers the s-string binding-strength form and the f-string{a:>10}form; it fails onmainand passes here.cargo test -p prqlc -p prqlc-parseris green, with no snapshot churn.task prqlc:pull-requestcouldn't run in the sandbox —cargo-instaisn't on the path there, which is what #6144 addresses — so CI is the first full-matrix run.