internal: remove dead code from the SQL backend - #6190
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Reviewing my own PR, so this is a COMMENT rather than an approval.
All three removals are behavior-neutral, verified locally on the merge commit: cargo test -p prqlc --lib (80 passed, 1 ignored) and --test integration (489 passed, 5 ignored) are green, compile --target sql.redshift on select {time = amount} still emits AS "time" (Redshift-only keyword still quoted), and group customer_id (take 1) still lowers to ROW_NUMBER() rather than DISTINCT ON.
One correction to the description, which I've already amended: the claim that the deleted ident_quoting_style comment described "behavior that isn't there" was wrong. Redshift is precisely the one dialect that has a per-dialect keyword set — dialect_keywords() in prqlc/prqlc/src/sql/keywords.rs matches Dialect::Redshift => redshift_keywords(), and the ConditionallyQuoted arm of translate_ident_part in sql/gen_expr.rs consults it through keywords::is_keyword(&ident, &ctx.dialect_enum); test_sql_keywords pins the distinction with is_keyword("time", &Dialect::Redshift) vs !is_keyword("time", &Dialect::Postgres). The override is still redundant — it returns the trait default, and the keyword behavior lives in keywords.rs regardless — so the deletion stands, but the reason is redundancy, not inaccuracy. The diff is unchanged; only the description was corrected.
Three dead-code removals in
prqlc/src/sql, found during the nightly rolling survey. No behavior change — each removal is either a comment, a no-op override, or a redundant wrapper.1. Stale commented-out
WITH RECURSIVEnesting intranslate_cte. ~30 commented-out lines sketched an alternative that wraps a non-first recursive CTE in a nestedWITH. It has drifted well past compiling: it buildssql_ast::Withwithoutwith_token,sql_ast::Ctewithoutmaterialized/closing_paren_token,TableFactor::Tablewithoutwith_ordinality/version/partitions/json_path/sample/index_hints, andsql_ast::ObjectName(vec![ident])from before theObjectNamePartchange — compare the live constructions a few lines above and below it. Anyone who did need this would be rewriting it against today'ssqlparserrather than uncommenting it. The four lines of prose explaining why the alternative exists are the part worth keeping, so those stay, with a pointer to git history for the sketch.2.
RedshiftDialect::ident_quoting_styleandRedshiftDialect::supports_distinct_onrestate the trait defaults.DialectHandler::ident_quoting_stylealready returnsConditionallyQuotedandsupports_distinct_onalready returnsfalse. Theident_quoting_styleoverride also carried a comment about "dialect-specific keywords". That behavior is real —dialect_keywords()insql/keywords.rsgives Redshift its own keyword set, and theConditionallyQuotedbranch oftranslate_ident_partconsults it viakeywords::is_keyword(&ident, &ctx.dialect_enum)— but it lives inkeywords.rs(covered bytest_sql_keywords) and doesn't depend on this override, which returns the trait default either way.3.
Some(&x).and_then(|x| map.get(x))intranslate_relation_expris justmap.get(&x).Verification
No new test: all three changes are deletions of code that never executed (a comment, two overrides returning the value the trait already returns, and an identity wrapper). The existing suite — in particular the
compileallsnapshots, which diff every dialect's output against generic, andtest_loop/ the recursive-CTE snapshots — covers the paths touched. A test asserting "Redshift quotes identifiers conditionally" would pass identically before and after, so it wouldn't be a regression test for anything.task prqlc:pull-requestcouldn't run here —cargo-instaisn't on the tend sandbox's PATH (that's what #6144 is for), so the plaincargo testruns above stand in. CI runs the full suite.