Conversation
…icking `RedshiftDialect::translate_chrono_item` maps every format specifier, so the format string translated fine, but `std.sql.prql` had no `redshift.date.to_text` to emit. `find_operator_impl` returned `None` and `translate_operator` unwrapped it, so `date.to_text` on `sql.redshift` aborted the compiler with `called \`Option::unwrap()\` on a \`None\` value`. Add the operator — Redshift's TO_CHAR takes the same Postgres-style format strings the dialect handler already emits — and replace the unwrap with the "operator is not supported for dialect" error the sibling `null`-body arm raises, so a future gap in the table reports rather than panics.
A query can declare its own `internal std.<name>`, so the operator lookup in `translate_operator` can come up empty on any target — it unwrapped the `None` and aborted the compiler. Return the same "operator is not supported for dialect" error the sibling `null`-body arm raises, and cover it.
date.to_text on the sql.redshift target instead of panicking
prql-bot
left a comment
There was a problem hiding this comment.
Reviewing my own PR, so this is a COMMENT rather than a verdict.
57febc07 answered the coverage question the better way round. I had it as unreachable defensive code — date.to_text being the only operator with no base implementation in std.sql.prql, and every dialect that gets past translate_chrono_item now having one. That misses that a query can declare its own internal std.<name>, which reaches the lookup on any target. So the arm is genuinely user-reachable, the new test earns its keep, and codecov/patch went green.
One claim survived the rewrite and is wrong: the changelog says Redshift "maps every format specifier". RedshiftDialect::translate_chrono_item ends in a catch-all returning PRQL doesn't support this format specifier, so a specifier as ordinary as %j fails on sql.redshift exactly as it does on sql.postgres. That sentence ships in the release notes, so it's worth being exact — Redshift covers the same specifiers Postgres does, not all of them.
A follow-up commit corrects it, and folds the else arm and the null-body arm below it into one unsupported_operator helper — they construct the same message verbatim, and both paths are now test-covered.
Checks behind this
$ echo 'prql target:sql.redshift
from invoices
select (invoice_date | date.to_text "%j")' | prqlc compile
Error: PRQL doesn't support this format specifier
$ echo 'prql target:sql.redshift
from invoices
select (invoice_date | date.to_text "%A %B %-d %Y")' | prqlc compile
SELECT TO_CHAR(invoice_date, 'FMDay FMMonth FMDD YYYY') FROM invoices
$ echo 'let my_op = column -> internal std.no_such_operator
from invoices
select (my_op total)' | prqlc compile
Error: operator std.no_such_operator is not supported for dialect generic
The book's dialect list was checked against the date.to_text declarations in std.sql.prql: bigquery, clickhouse, duckdb, mssql, mysql, postgres and now redshift — seven, matching the note. The dialects without one (generic, ansi, sqlite, snowflake, oracle) fail earlier with Date formatting is not yet supported for this dialect, so none of them regress.
At this head, cargo test -p prqlc --lib (94 passed) and --test integration (509 passed) are green, as is cargo clippy -p prqlc --all-targets --no-default-features --features=default,lsp -- -D warnings. That clippy run covers the default,lsp feature set only, not the matrix's test-dbs-external.
`RedshiftDialect::translate_chrono_item` ends in a catch-all returning "PRQL doesn't support this format specifier", so `%j` fails on `sql.redshift` just as it does on `sql.postgres`. The changelog claimed Redshift maps every format specifier; it maps the same ones Postgres does. The missing-implementation arm and the `null`-body arm built the same message verbatim, so both now call `unsupported_operator`.
prql-bot
left a comment
There was a problem hiding this comment.
Reviewing my own PR, so this is a COMMENT rather than a verdict.
The changelog reads correctly now — bd018577 scopes the Redshift claim to Postgres's specifier set, which matches the two dialects' translate_chrono_item bodies being identical specifier for specifier.
One finding, in the same function. The construct the new unknown_internal_operator_is_reported test uses reaches a second unwrap a few lines below the one this PR fixes, and that one still aborts the compiler. translate_operator zips the implementation's parameter names against the arguments it was passed, then looks each interpolated name up in the resulting map — and a let binding may declare its own internal std.<name> with fewer parameters than the implementation's body reads, so the lookup comes up empty:
$ echo 'let my_op = column -> internal std.lag
from invoices
select (my_op total)' | prqlc compile
The application panicked (crashed).
Message: called `Option::unwrap()` on a `None` value
Location: prqlc/prqlc/src/sql/operators.rs:82
std.lag is declared offset column, the binding passes one argument, zip truncates the map to offset, and the body's {column:0} finds nothing. That is the same reachability that earned this PR's own test, so a follow-up commit makes the lookup return operator std.lag expects 2 arguments, found 1 against the call site's span, adds a test beside the two already there, and widens the changelog entry to cover it. CLAUDE.md's Error Handling section is explicit that user input must not panic.
Left alone deliberately, and a maintainer's call rather than mine: the mirror case drops arguments silently instead of panicking. let my_op = a b -> internal std.min compiles select (my_op total quantity) to MIN(total), with zip discarding quantity and no diagnostic. A strict arity equality would catch both directions, but it would also reject any query where the compiler itself passes more arguments than an implementation's body reads, and I have no evidence either way about whether such a path exists — so the pushed fix only converts the panic.
Checks behind this
The panic reproduces at bd018577 and not after the follow-up; both forms above were run through a debug prqlc built from this branch. %d/%m/%Y on sql.redshift compiles to TO_CHAR(invoice_date, 'DD/MM/YYYY'), unchanged by the follow-up.
The Postgres and Redshift DialectHandler impls were diffed against each other in dialect.rs: their translate_chrono_item arms differ only in a comment naming the dialect, so "the same format specifiers as Postgres" is exact. The book's note lists seven dialects; std.sql.prql declares date.to_text in exactly seven modules — bigquery, clickhouse, duckdb, mssql, mysql, postgres, redshift.
At the follow-up head, cargo test -p prqlc --lib (95 passed) and --test integration (509 passed) are green, as are cargo fmt --all --check and cargo clippy -p prqlc --all-targets --no-default-features --features=default,lsp -- -D warnings. That clippy run covers the default,lsp feature set only, not the matrix's test-dbs-external.
#6351 touches CHANGELOG.md and web/book/src/reference/stdlib/date.md as well, but a different date.md hunk and a different bug — naming the rejected format specifier — so it is complementary, not a duplicate.
prql-bot
left a comment
There was a problem hiding this comment.
Reviewing my own PR, so this is a COMMENT rather than a verdict.
2b64c3c9 closes the arity panic, and the fix is now complete for the function it targets. I swept every operator declared in std.sql.prql — 73 names — through the internal std.<name> hatch at one, two and three arguments: no combination reaches a panic inside translate_operator any more, each either compiles or reports.
Five of those 219 combinations still panic, but earlier, in files this PR doesn't touch:
$ echo 'let my_op = a -> internal std.eq
from invoices
select (my_op total)' | prqlc compile
The application panicked (crashed).
Message: index out of bounds: the len is 1 but the index is 1
Location: prqlc/prqlc/src/semantic/resolver/static_eval.rs:40
static_eval_rq_operator indexes args[0] and args[1] unconditionally in its std.eq, std.ne, std.and and std.or arms, so a one-argument declaration panics during resolution. std.and at three arguments gets past it and lands on try_into_between's let [a, b]: [_; 2] = args.try_into().unwrap() in gen_expr.rs.
Same bug class, and CLAUDE.md's Error Handling section covers them as squarely as it covers the one this PR fixed — but I'm not folding them in, because the fix isn't derivable without a decision that's already open. static_eval_rq_operator returns Expr, not Result, so the natural shape is to skip constant-folding on a mismatched arity and let a later stage report. Do that, and std.and with three arguments flows past try_into_between into translate_operator, where zip truncates and emits a AND b with the third argument silently dropped — the mirror case I flagged last round as a maintainer's call. Converting a panic into silently wrong SQL is worse than the panic, so the two want one answer.
One cleanup in the new code, as an inline suggestion. Not pushing it: it's cosmetic, the branch is green across the full matrix including default,test-dbs-external,lsp, and a CI round for a dropped allocation isn't worth it.
Checks behind this
The sweep extracted every let <name> = binding from std.sql.prql, stripped the dialect module prefix, and compiled let my_op = <params> -> internal std.<name> against select (my_op ...) at arities 1–3 with a debug prqlc built from this branch, matching stderr for panicked. Five hits, all listed above; zero in operators.rs.
Two shapes were checked and are not reachable. find_operator_impl's operator_name.strip_prefix("std.").unwrap() can't fire: fold_function only builds an RqOperator when the internal name starts_with("std."), and routes everything else to resolve_special_func, which reports unknown operator foo.no_such — confirmed by compiling internal foo.no_such and bare internal std. And the new redshift.date module doesn't shadow the base date module's other entries: date.diff on sql.redshift still compiles to DATEDIFF(days, a, b), since the lookup is by full date.diff path and falls back to std when the dialect module has no such member.
unsupported_operator's format string is byte-identical to the two inline Error::new_simple calls it replaced, so neither message changed.
At 88f8f504: cargo test -p prqlc --lib (95 passed) and --test integration (509 passed) are green, as are cargo fmt --all --check and cargo clippy -p prqlc --all-targets --no-default-features --features=default,lsp -- -D warnings. That clippy run covers default,lsp only; the matrix's test-rust (x86_64-unknown-linux-gnu, ubuntu-24.04, default,test-dbs-external,lsp) job is green on this head and covers the rest. The suggested cleanup was applied locally and re-verified against the same four commands before being posted.
| .map(|x| x.name.split('.').next_back().unwrap_or(x.name.as_str())) | ||
| .collect_vec(); | ||
|
|
||
| let param_count = params.len(); |
There was a problem hiding this comment.
params is only collected so params.len() can be read back — the count is already available from the two fields the chain iterates, and zip takes the lazy iterator just as it did before. Dropping the collect_vec removes a heap allocation from every operator translation and restores the original three-line shape.
| .map(|x| x.name.split('.').next_back().unwrap_or(x.name.as_str())) | |
| .collect_vec(); | |
| let param_count = params.len(); | |
| .map(|x| x.name.split('.').next_back().unwrap_or(x.name.as_str())); | |
| let param_count = func_def.named_params.len() + func_def.params.len(); |
…pens with a finding (#1362) Self-review COMMENTs open with "Reviewing my own PR, so this is a COMMENT rather than a verdict." (PRQL/prql#6352 has it in three rounds). No instruction contains or asks for the line. The first round produced it by chance; in 12 replays of that session it came up in none. Later rounds copy it, because each reads the earlier bot reviews in full for dedup and follows their form. The Submit step now says GitHub labels the review approved or commented above its body, so the body opens with the first finding. It applies to every review: naming the self-authored case in the prompt raised the rate in a synthetic A/B. Replaying that PR's three review sessions, cut just before each wrote its body: | Skill text | Round 2 (one earlier copy) | Round 3 (two earlier copies) | |---|---|---| | Before | 4/4 | 4/4 | | This PR | 1/8, moved down from the opening | 7/7 | Nothing tried stops round 3, including an explicit prohibition: once two posted reviews carry a line, the session copies it. The second commit backticks the event names (`COMMENT`, `APPROVE`) in skill prose, to match the review states (`APPROVED`, `CHANGES_REQUESTED`) already backticked beside them. On its own it doesn't change the opener (round 2 stayed at 4/4). > _This was written by Claude Code on behalf of max-sixty_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
translate_operatorlooked up an operator's implementation for the target dialect and unwrapped the result, so a lookup that came up empty aborted the compiler rather than reporting. Two queries reach it:Redshift is the only target in the shipped operator table that gets this far.
RedshiftDialect::translate_chrono_itemmirrors Postgres's, specifier for specifier — it was added alongside it — so the format string translates cleanly and compilation proceeds to emitting SQL. Butstd.sql.prqlhad noredshift.date.to_text, anddate.to_textis the one operator with no base implementation to fall back on. Every other target without it (generic,sqlite,snowflake,oracle) fails earlier, intranslate_chrono_item, with a proper error. The second query needs no gap in the table at all: aletbinding may declare aninternal std.<name>of its own, and an unknown name lands on the same unwrap on every target.Two changes:
null-body arm already raises, instead of unwrapping. The second query now reportsoperator std.no_such_operator is not supported for dialect genericagainst the call site's span.redshift.date.to_text, so the first query compiles rather than merely erroring more politely. Redshift'sTO_CHARtakesTO_CHAR(timestamp_expression, 'format')with the same Postgres-style format strings the dialect handler already emits, down to theFMprefixes andUSmicroseconds, so it compiles toTO_CHAR(invoice_date, 'DD/MM/YYYY').Both queries are covered by tests in
operators.rs; each panics onmain. The book's list of dialects supportingdate.to_textgains Redshift to match.