docs(sql): complete the window-function list, and guard it with a test - #246
Merged
Conversation
The "Supported window functions include:" list in dql_statements.md had fallen four entries behind the parser. `identifierWithWindowFunction` accepts 14 forms; the list named 10, omitting AVG, MIN, MAX and PERCENTILE_CONT/PERCENTILE_DISC. The reference page (functions_aggregate.md) documents all of them, so this was the enumerated list going stale, not a missing feature -- but the DQL page is where a reader looks to find out what works with OVER. Every added form was run through the real parser before being written down, and each also round-trips: statement.sql parses again. The percentile forms revealed something worth documenting on its own -- all four spellings (OVER, WITHIN GROUP, both, and the (column, p) shorthand) normalize to the same canonical rendering, so a statement round-tripped through the engine comes back in a syntax the user did not type. WindowFunctionCoverageSpec locks the surface down: 26 documented forms, each asserted to parse and to re-parse from its own rendering, plus 3 rejections the docs state explicitly (a ranking window with no ORDER BY, and a percentile fraction outside [0,1]). Nothing asserted the documented surface before, which is why the list could drift unnoticed.
…g it guards Independent review of the first commit; every finding below was confirmed against the real parser before being acted on. The test was weaker than its own scaladoc claimed. `Parser(rendered).isRight` proves a rendering parses, not that it is faithful -- and the suite already contained a case that proves the difference: ARRAY_AGG's inline LIMIT is parsed, kept on the AST, and silently dropped when rendering (emitsLimitInOver is false for everything but ranking windows). The old assertion was green on exactly the corruption it was written to catch. Every form is now pinned to its exact rendering, which is the only assertion that catches a dropped clause. That also turns the PR's headline finding into something asserted rather than merely asserted-in-prose: the five percentile spellings are each pinned to the canonical WITHIN GROUP form they normalize to. The ARRAY_AGG loss gets its own named test so the day it is fixed, the suite says so instead of staying quietly green. Negative tests now assert WHY. `ROW_NUMBER() OVER (PARTITION BY d)` and `ROW_NUMBER() AS rn` are rejected with the SAME generic message, so isLeft alone could not tell "the ANSI rule fired" from "ROW_NUMBER stopped being a window function" -- it is now paired with the ORDER BY variant that must parse. The percentile rejections assert the message names the [0,1] bound. Docs: `PERCENTILE_DISC(p) OVER (...)` was listed but does not parse -- the value column is required, so it needs ORDER BY inside OVER. SUM/AVG/MIN/ MAX/COUNT now show `OVER (PARTITION BY ...)` rather than `OVER (...)`, because an ORDER BY inside a non-ranking OVER is accepted and then discarded. The percentile "See" link pointed at another page while this file has its own percentile section; it now points at both. Also: scalafmt. The first commit did NOT pass sql/Test/scalafmtCheck -- verified by restoring it and running the gate -- so CI would have failed. 32 tests green; 532 sql tests green; scalafmtCheck green.
The known-loss test carried a dangling 'See SoftClient4ES issue' with no number. #247 now records it: the render drops the inline LIMIT, and ArrayAgg.update falls back to request.limit, so the statement's own LIMIT is substituted for the inline one on the next parse -- a different answer rather than an error.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
documentation/sql/dql_statements.mdenumerates which functions work withOVER. That list had fallen four entries behind the parser:identifierWithWindowFunctionaccepts 14 forms, the list named 10 — omittingAVG,MIN,MAXandPERCENTILE_CONT/PERCENTILE_DISC.The reference page (
functions_aggregate.md) documents all of them, and so dokeywords.mdand the REPL help JSON. So this was an enumerated list going stale, not a missing feature — but the DQL page is where a reader goes to find out what works withOVER, and it was the one saying no.Found because the same omission had propagated into a blog post.
Verified, not read
Every added form was run through the real parser before being written down, and each is asserted to round-trip —
statement.sqlmust parse again. A round-trip that silently drops a clause is how a documented form becomes a wrong query downstream.The percentile forms turned up something worth documenting on its own: all four spellings normalize to the same canonical rendering. Writing
gets you back
Same for the
(column, p)shorthand. A statement round-tripped through the engine returns in a syntax the user did not type — that is now stated in the docs.The test
WindowFunctionCoverageSpec— 32 tests, green (plus 532sqltests andscalafmtCheck).Every form is pinned to its exact rendering, not to "it parses". That distinction is the whole point, and the first revision of this PR got it wrong:
Parser(rendered).isRightproves a rendering parses, not that it is faithful — and the suite already contained the counter-example.ARRAY_AGG's inlineLIMITis parsed, kept on the AST, and silently dropped when rendering (emitsLimitInOverisfalsefor everything but ranking windows). The weaker assertion was green on precisely the corruption it was written to catch.ARRAY_AGGLIMITloss, so the day it is fixed the suite says so instead of staying quietly greenROW_NUMBER() OVER (PARTITION BY d)andROW_NUMBER() AS rnare rejected with the same generic message, soisLeftalone could not tell "the ANSI rule fired" from "ROW_NUMBERstopped being a window function"; it is now paired with theORDER BYvariant that must parse. The percentile rejections assert the message names the[0,1]bound.Corrections from review
PERCENTILE_DISC(p) OVER (...)was listed and does not parse — the value column is required, soORDER BYinsideOVERis not optionalSUM/AVG/MIN/MAX/COUNTnow showOVER (PARTITION BY ...): anORDER BYinside a non-rankingOVERis accepted and then discarded (COUNT(x) OVER (PARTITION BY c ORDER BY ts)renders back without it)sql/Test/scalafmtCheck— verified by restoring it and running the gateNot included
softclient4es-web'ssrc/content/docs/sql/dql.mdxcarries the same section and the same gap. That is a separate PR on the web repo.🤖 Generated with Claude Code