Skip to content

docs: fix invalid DISTINCT ON SQL in the s-strings book page - #6248

Merged
max-sixty merged 1 commit into
mainfrom
docs/s-string-distinct-on
Aug 30, 2026
Merged

docs: fix invalid DISTINCT ON SQL in the s-strings book page#6248
max-sixty merged 1 commit into
mainfrom
docs/s-string-distinct-on

Conversation

@prql-bot

@prql-botprql-bot commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

The "we can also use s-strings to produce a full table" example in the s-strings reference page embeds SQL that no database accepts. PostgreSQL's grammar is SELECT DISTINCT ON ( expression [, ...] ) select_list, so the parenthesised expression list is required; the example writes SELECT DISTINCT ON first_name, id, age, which is a syntax error. It also needs ORDER BY to start with the DISTINCT ON expressions. This corrects both, keeping the example's dialect-specific escape-hatch flavour intact.

Verified against DuckDB (which implements PostgreSQL's DISTINCT ON): the current text fails with Parser Error: syntax error at or near "first_name", the corrected text parses.

A nice side effect: the invalid SQL was also degrading PRQL's own inference. prqlc reads column names out of an s-string relation, and the unparseable text made it give up and emit table_0.*. With valid SQL it recovers the column set, so the book's compiled output now enumerates the columns as table_0.id, table_0.age, table_0.first_name — that snapshot change is the compiler behaving better, not a regression. The emitted order isn't the s-string's: id leads as the already-inferred join column, and the extracted names follow alphabetically because try_extract_sql_columns collects them into a BTreeSet. That ordering is pre-existing and untouched here.

No regression test beyond the snapshot: the book examples are themselves compiled and snapshotted by web/book/tests/documentation/book.rs, so the updated .snap is the test. Validity of the embedded SQL isn't something the test suite can check, since an s-string is opaque passthrough by design.

Verification
$ python -c "import duckdb; ..."
FAIL | book (current) -> Parser Error: syntax error at or near "first_name"
OK | parens + matching ORDER BY

Snapshot regenerated with INSTA_UPDATE=always cargo test -p mdbook-prql --test documentation, then re-checked with a plain cargo test -p mdbook-prql --test documentation — 7 passed. That second run is a real assertion rather than a rewrite: CI is set in this environment, so insta's default auto resolves to NoUpdate. Only documentation__book__reference__syntax__s-strings__3.snap is included. Three other snapshots have stale expression: headers that a forced update also rewrites — those are unrelated pre-existing drift and are deliberately left out of this PR.

@prql-botprql-bot left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SQL correction is right, and it's the only from s"..." full-table example in the book, so there's nothing else carrying the same defect. Two notes, neither blocking.

The description's column list doesn't match the snapshot. The body says the compiler "recovers first_name, id, age", but the regenerated snapshot emits table_0.id, table_0.age, table_0.first_name. The set is recovered; the order isn't the s-string's. Worth rewording so a reader comparing the two doesn't stall on it.

Where that order comes from. In try_extract_sql_columns (prqlc/prqlc/src/semantic/lowering.rs), the extracted names are collected as .collect::<BTreeSet<String>>() under the comment // deduplicate extracted columns, but preserve their order — a BTreeSet sorts, so the comment and the type disagree. The emitted order is the already-inferred column (id, from the (==id) join) followed by the alphabetically sorted extraction (age, first_name), which reproduces the snapshot exactly. That's pre-existing and out of scope for a docs fix — swapping in an order-preserving set would move snapshots across the repo — but it's the reason the book now shows an output order that looks arbitrary, and it's worth a separate look.

Verification notes
  • Confirmed the inference path is real, not incidental: pl::ExprKind::SString lowering calls try_extract_sql_columns, which parses the s-string with sqlparser's GenericDialect and bails to the original (wildcard) columns on a parse error — so the previously invalid text is exactly why the old snapshot emitted table_0.*.
  • SELECT * FROM salaries still hits the has_wildcard branch, which is why table_1.* is unchanged.
  • Line length on the lengthened markdown line is not a lint concern: MD013 is disabled in .markdownlint-cli2.yaml, and prettier doesn't reflow inside code fences.
  • I did not rebuild the book snapshot locally; test-rust (x86_64-unknown-linux-gnu, …, default,test-dbs-external,lsp) covers it and was still running when this was written.

@prql-bot

Copy link
Copy Markdown
CollaboratorAuthor

Reworded the description — it now says the column set is recovered and gives the emitted order verbatim (table_0.id, table_0.age, table_0.first_name), with a one-line note on where that order comes from, so nobody stalls comparing it against the snapshot. No code change; the diff is unchanged.

On the BTreeSet: confirmed at lowering.rs#L1116-L1117 — the comment says "preserve their order" and the type sorts. Leaving it here rather than opening a second PR: the honest fix is a choice between correcting the comment and swapping in an order-preserving set, and only the second one is interesting (it changes emitted column order for every s-string relation, moving snapshots beyond this crate). That's a maintainer call, not something to fold into a docs fix.

The verification gap the review flagged is closed: test-rust (x86_64-unknown-linux-gnu, ubuntu-24.04, default,test-dbs-external,lsp) finished green on be44fcd, which is the job that compiles the book snapshots.

@max-sixty
max-sixty merged commit 722b035 into mainAug 30, 2026
44 checks passed
@max-sixty
max-sixty deleted the docs/s-string-distinct-on branch August 30, 2026 05:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@prql-bot@max-sixty