From eb76eddabbbca65e13d93c7d26f117710ac8890a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Wed, 26 Aug 2026 20:43:08 +0200 Subject: [PATCH 1/3] docs(sql): complete the window-function list, and guard it with a test 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. --- documentation/sql/dql_statements.md | 11 ++- .../parser/WindowFunctionCoverageSpec.scala | 82 +++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 527fe11e..323b0377 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -412,7 +412,9 @@ Window functions operate over a logical window of rows defined by `OVER (PARTITI Supported window functions include: - `SUM(expr) OVER (...)` -- `COUNT(expr) OVER (...)` +- `AVG(expr) OVER (...)` +- `MIN(expr) OVER (...)` / `MAX(expr) OVER (...)` +- `COUNT(expr) OVER (...)`, including `COUNT(DISTINCT expr) OVER (...)` - `STDDEV(expr) OVER (PARTITION BY ...)` and its `_SAMP` / `_POP` variants - `VARIANCE(expr) OVER (PARTITION BY ...)` and its `_SAMP` / `_POP` variants - `FIRST_VALUE(expr) OVER (...)` @@ -421,6 +423,13 @@ Supported window functions include: - `ROW_NUMBER() OVER ([PARTITION BY ...] ORDER BY ...)` - `RANK() OVER ([PARTITION BY ...] ORDER BY ...)` - `DENSE_RANK() OVER ([PARTITION BY ...] ORDER BY ...)` +- `PERCENTILE_CONT(p) OVER (PARTITION BY ... ORDER BY column)` and `PERCENTILE_DISC(p) OVER (...)` + +`PERCENTILE_CONT` / `PERCENTILE_DISC` accept four equivalent spellings — the `OVER (... ORDER BY column)` +form above, `WITHIN GROUP (ORDER BY column)`, the two combined, and the `(column, p)` shorthand. All four +**normalize to the same canonical rendering**, `PERCENTILE_CONT(p) WITHIN GROUP (ORDER BY column) [OVER +(PARTITION BY ...)]`, so a statement round-tripped through the engine comes back in that form rather than +the one you typed. See [Aggregate Functions](functions_aggregate.md#function-percentile_cont--percentile_disc). #### Basic window example diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala new file mode 100644 index 00000000..01ddb58a --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala @@ -0,0 +1,82 @@ +package app.softnetwork.elastic.sql.parser + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** Guards the window-function surface the documentation promises. + * + * `documentation/sql/dql_statements.md` enumerates which functions work with `OVER`, and that list + * silently fell four entries behind the parser (`AVG`, `MIN`, `MAX`, `PERCENTILE_CONT/DISC`) — + * reading the code did not catch it, and neither did any test, because nothing asserted the + * documented surface. Every form below is one the docs tell a user to write. + * + * Two properties per form, because parsing alone is not enough: the statement must parse, and its + * rendered `.sql` must parse again. A round-trip that silently drops a clause is how a documented + * form turns into a wrong query downstream. + * + * When a window function is added, add its documented spellings here and to `dql_statements.md`. + */ +class WindowFunctionCoverageSpec extends AnyFlatSpec with Matchers { + + /** label -> statement, one per documented spelling. */ + private val documented: Seq[(String, String)] = Seq( + "SUM" -> "SELECT product, SUM(amount) OVER (PARTITION BY product) AS s FROM sales", + "AVG" -> "SELECT category, AVG(price) OVER (PARTITION BY category) AS a FROM products", + "MIN" -> "SELECT brand, MIN(price) OVER (PARTITION BY brand) AS mn FROM products", + "MAX" -> "SELECT brand, MAX(price) OVER (PARTITION BY brand) AS mx FROM products", + "COUNT star" -> "SELECT customer_id, COUNT(*) OVER (PARTITION BY customer_id) AS c FROM orders", + "COUNT DISTINCT" -> "SELECT country, COUNT(DISTINCT city) OVER (PARTITION BY country) AS c FROM places", + "FIRST_VALUE" -> "SELECT product, FIRST_VALUE(amount) OVER (PARTITION BY product ORDER BY ts ASC) AS f FROM sales", + "LAST_VALUE" -> "SELECT product, LAST_VALUE(amount) OVER (PARTITION BY product ORDER BY ts ASC) AS l FROM sales", + "ARRAY_AGG" -> "SELECT product, ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC) AS t FROM sales", + "ARRAY_AGG inline LIMIT" -> "SELECT product, ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC LIMIT 10) AS t FROM sales", + "STDDEV" -> "SELECT department, STDDEV(salary) OVER (PARTITION BY department) AS sd FROM employees", + "STDDEV_SAMP" -> "SELECT department, STDDEV_SAMP(salary) OVER (PARTITION BY department) AS sd FROM employees", + "STDDEV_POP" -> "SELECT department, STDDEV_POP(salary) OVER (PARTITION BY department) AS sd FROM employees", + "VARIANCE" -> "SELECT department, VARIANCE(salary) OVER (PARTITION BY department) AS v FROM employees", + "VAR_SAMP" -> "SELECT department, VAR_SAMP(salary) OVER (PARTITION BY department) AS v FROM employees", + "VAR_POP" -> "SELECT department, VAR_POP(salary) OVER (PARTITION BY department) AS v FROM employees", + // all four percentile spellings are documented; they normalize to one canonical rendering + "PERCENTILE_CONT OVER" -> "SELECT service, PERCENTILE_CONT(0.95) OVER (PARTITION BY service ORDER BY ms) AS p95 FROM logs", + "PERCENTILE_DISC OVER" -> "SELECT service, PERCENTILE_DISC(0.95) OVER (PARTITION BY service ORDER BY ms) AS p95 FROM logs", + "PERCENTILE WITHIN GROUP" -> "SELECT PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) AS p95 FROM logs", + "PERCENTILE WITHIN + OVER" -> "SELECT service, PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) OVER (PARTITION BY service) AS p95 FROM logs", + "PERCENTILE shorthand" -> "SELECT PERCENTILE_CONT(ms, 0.95) AS p95 FROM logs", + "ROW_NUMBER" -> "SELECT name, ROW_NUMBER() OVER (PARTITION BY department ORDER BY salary DESC) AS rn FROM employees", + "RANK" -> "SELECT name, RANK() OVER (PARTITION BY department ORDER BY salary DESC) AS rk FROM employees", + "DENSE_RANK" -> "SELECT name, DENSE_RANK() OVER (PARTITION BY department ORDER BY salary DESC) AS dr FROM employees", + "RANK inline LIMIT" -> "SELECT name, RANK() OVER (PARTITION BY department ORDER BY salary DESC LIMIT 3) AS rk FROM employees", + "ranking without PARTITION BY" -> "SELECT name, ROW_NUMBER() OVER (ORDER BY salary DESC) AS rn FROM employees" + ) + + documented.foreach { case (label, sql) => + it should s"parse the documented window form: $label" in { + Parser(sql) match { + case Left(err) => fail(s"documented form failed to parse: $sql -> $err") + case Right(statement) => + val rendered = statement.sql + withClue(s"rendered SQL does not re-parse: $rendered\n") { + Parser(rendered).isRight shouldBe true + } + } + } + } + + /** Rejections the documentation states explicitly; they must stay rejections. */ + private val rejected: Seq[(String, String)] = Seq( + // ANSI: a ranking with no order is not a ranking. Rejected at parse time on purpose, rather + // than parsing and breaking at execution. + "ranking window without ORDER BY" -> + "SELECT name, ROW_NUMBER() OVER (PARTITION BY department) AS rn FROM employees", + "percentile fraction above 1" -> + "SELECT PERCENTILE_CONT(1.5) WITHIN GROUP (ORDER BY ms) AS p FROM logs", + "percentile fraction below 0" -> + "SELECT PERCENTILE_CONT(-0.5) WITHIN GROUP (ORDER BY ms) AS p FROM logs" + ) + + rejected.foreach { case (label, sql) => + it should s"reject: $label" in { + Parser(sql).isRight shouldBe false + } + } +} From 4f41f1d77d35420c4be1365e163128b00333758e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Wed, 26 Aug 2026 21:13:19 +0200 Subject: [PATCH 2/3] fix(sql): review pass -- the round-trip assertion was green on the bug 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. --- documentation/sql/dql_statements.md | 12 +- .../parser/WindowFunctionCoverageSpec.scala | 189 ++++++++++++------ 2 files changed, 133 insertions(+), 68 deletions(-) diff --git a/documentation/sql/dql_statements.md b/documentation/sql/dql_statements.md index 323b0377..347de1c7 100644 --- a/documentation/sql/dql_statements.md +++ b/documentation/sql/dql_statements.md @@ -411,10 +411,10 @@ Window functions operate over a logical window of rows defined by `OVER (PARTITI Supported window functions include: -- `SUM(expr) OVER (...)` -- `AVG(expr) OVER (...)` -- `MIN(expr) OVER (...)` / `MAX(expr) OVER (...)` -- `COUNT(expr) OVER (...)`, including `COUNT(DISTINCT expr) OVER (...)` +- `SUM(expr) OVER (PARTITION BY ...)` +- `AVG(expr) OVER (PARTITION BY ...)` +- `MIN(expr) OVER (PARTITION BY ...)` / `MAX(expr) OVER (PARTITION BY ...)` +- `COUNT(expr) OVER (PARTITION BY ...)`, including `COUNT(DISTINCT expr) OVER (PARTITION BY ...)` - `STDDEV(expr) OVER (PARTITION BY ...)` and its `_SAMP` / `_POP` variants - `VARIANCE(expr) OVER (PARTITION BY ...)` and its `_SAMP` / `_POP` variants - `FIRST_VALUE(expr) OVER (...)` @@ -423,13 +423,13 @@ Supported window functions include: - `ROW_NUMBER() OVER ([PARTITION BY ...] ORDER BY ...)` - `RANK() OVER ([PARTITION BY ...] ORDER BY ...)` - `DENSE_RANK() OVER ([PARTITION BY ...] ORDER BY ...)` -- `PERCENTILE_CONT(p) OVER (PARTITION BY ... ORDER BY column)` and `PERCENTILE_DISC(p) OVER (...)` +- `PERCENTILE_CONT(p) OVER (PARTITION BY ... ORDER BY column)` and `PERCENTILE_DISC(p) OVER (PARTITION BY ... ORDER BY column)` `PERCENTILE_CONT` / `PERCENTILE_DISC` accept four equivalent spellings — the `OVER (... ORDER BY column)` form above, `WITHIN GROUP (ORDER BY column)`, the two combined, and the `(column, p)` shorthand. All four **normalize to the same canonical rendering**, `PERCENTILE_CONT(p) WITHIN GROUP (ORDER BY column) [OVER (PARTITION BY ...)]`, so a statement round-tripped through the engine comes back in that form rather than -the one you typed. See [Aggregate Functions](functions_aggregate.md#function-percentile_cont--percentile_disc). +the one you typed. See [Percentiles](#percentiles--percentile_cont--percentile_disc) below for the spellings themselves, and [Aggregate Functions](functions_aggregate.md#function-percentile_cont--percentile_disc) for the full reference. #### Basic window example diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala index 01ddb58a..41852183 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala @@ -3,80 +3,145 @@ package app.softnetwork.elastic.sql.parser import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers -/** Guards the window-function surface the documentation promises. +/** Guards the window-function surface `documentation/sql/dql_statements.md` promises. * - * `documentation/sql/dql_statements.md` enumerates which functions work with `OVER`, and that list - * silently fell four entries behind the parser (`AVG`, `MIN`, `MAX`, `PERCENTILE_CONT/DISC`) — - * reading the code did not catch it, and neither did any test, because nothing asserted the - * documented surface. Every form below is one the docs tell a user to write. + * That list silently fell four entries behind the parser (`AVG`, `MIN`, `MAX`, + * `PERCENTILE_CONT`/`PERCENTILE_DISC`). Reading the code did not catch it and no test could, + * because nothing asserted the documented surface. * - * Two properties per form, because parsing alone is not enough: the statement must parse, and its - * rendered `.sql` must parse again. A round-trip that silently drops a clause is how a documented - * form turns into a wrong query downstream. + * Each form is pinned to its **exact rendering**, not merely to "parses". + * `Parser(rendered).isRight` alone is worth little here: a rendering that silently drops a clause + * still parses, so the weaker assertion is green on precisely the corruption it looks like it + * guards. Pinning the string makes a dropped clause a failing diff — and doubles as executable + * documentation of what the engine gives back, which is not always what the caller wrote. * - * When a window function is added, add its documented spellings here and to `dql_statements.md`. + * Adding a window function means adding its documented spellings here and to `dql_statements.md`. */ class WindowFunctionCoverageSpec extends AnyFlatSpec with Matchers { - /** label -> statement, one per documented spelling. */ - private val documented: Seq[(String, String)] = Seq( - "SUM" -> "SELECT product, SUM(amount) OVER (PARTITION BY product) AS s FROM sales", - "AVG" -> "SELECT category, AVG(price) OVER (PARTITION BY category) AS a FROM products", - "MIN" -> "SELECT brand, MIN(price) OVER (PARTITION BY brand) AS mn FROM products", - "MAX" -> "SELECT brand, MAX(price) OVER (PARTITION BY brand) AS mx FROM products", - "COUNT star" -> "SELECT customer_id, COUNT(*) OVER (PARTITION BY customer_id) AS c FROM orders", - "COUNT DISTINCT" -> "SELECT country, COUNT(DISTINCT city) OVER (PARTITION BY country) AS c FROM places", - "FIRST_VALUE" -> "SELECT product, FIRST_VALUE(amount) OVER (PARTITION BY product ORDER BY ts ASC) AS f FROM sales", - "LAST_VALUE" -> "SELECT product, LAST_VALUE(amount) OVER (PARTITION BY product ORDER BY ts ASC) AS l FROM sales", - "ARRAY_AGG" -> "SELECT product, ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC) AS t FROM sales", - "ARRAY_AGG inline LIMIT" -> "SELECT product, ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC LIMIT 10) AS t FROM sales", - "STDDEV" -> "SELECT department, STDDEV(salary) OVER (PARTITION BY department) AS sd FROM employees", - "STDDEV_SAMP" -> "SELECT department, STDDEV_SAMP(salary) OVER (PARTITION BY department) AS sd FROM employees", - "STDDEV_POP" -> "SELECT department, STDDEV_POP(salary) OVER (PARTITION BY department) AS sd FROM employees", - "VARIANCE" -> "SELECT department, VARIANCE(salary) OVER (PARTITION BY department) AS v FROM employees", - "VAR_SAMP" -> "SELECT department, VAR_SAMP(salary) OVER (PARTITION BY department) AS v FROM employees", - "VAR_POP" -> "SELECT department, VAR_POP(salary) OVER (PARTITION BY department) AS v FROM employees", - // all four percentile spellings are documented; they normalize to one canonical rendering - "PERCENTILE_CONT OVER" -> "SELECT service, PERCENTILE_CONT(0.95) OVER (PARTITION BY service ORDER BY ms) AS p95 FROM logs", - "PERCENTILE_DISC OVER" -> "SELECT service, PERCENTILE_DISC(0.95) OVER (PARTITION BY service ORDER BY ms) AS p95 FROM logs", - "PERCENTILE WITHIN GROUP" -> "SELECT PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) AS p95 FROM logs", - "PERCENTILE WITHIN + OVER" -> "SELECT service, PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) OVER (PARTITION BY service) AS p95 FROM logs", - "PERCENTILE shorthand" -> "SELECT PERCENTILE_CONT(ms, 0.95) AS p95 FROM logs", - "ROW_NUMBER" -> "SELECT name, ROW_NUMBER() OVER (PARTITION BY department ORDER BY salary DESC) AS rn FROM employees", - "RANK" -> "SELECT name, RANK() OVER (PARTITION BY department ORDER BY salary DESC) AS rk FROM employees", - "DENSE_RANK" -> "SELECT name, DENSE_RANK() OVER (PARTITION BY department ORDER BY salary DESC) AS dr FROM employees", - "RANK inline LIMIT" -> "SELECT name, RANK() OVER (PARTITION BY department ORDER BY salary DESC LIMIT 3) AS rk FROM employees", - "ranking without PARTITION BY" -> "SELECT name, ROW_NUMBER() OVER (ORDER BY salary DESC) AS rn FROM employees" - ) + behavior of "the documented window-function surface" - documented.foreach { case (label, sql) => - it should s"parse the documented window form: $label" in { - Parser(sql) match { - case Left(err) => fail(s"documented form failed to parse: $sql -> $err") - case Right(statement) => - val rendered = statement.sql - withClue(s"rendered SQL does not re-parse: $rendered\n") { - Parser(rendered).isRight shouldBe true - } + private val select = "SELECT " + private val from = " FROM t" + + /** (label, written, rendered-back). `rendered` differing from `written` is a documented fact. */ + private val documented: Seq[(String, String, String)] = { + def same(l: String, sql: String) = (l, sql, sql) + Seq( + same("SUM", "SUM(amount) OVER (PARTITION BY product) AS s"), + same("AVG", "AVG(price) OVER (PARTITION BY category) AS a"), + same("MIN", "MIN(price) OVER (PARTITION BY brand) AS mn"), + same("MAX", "MAX(price) OVER (PARTITION BY brand) AS mx"), + same("COUNT star", "COUNT(*) OVER (PARTITION BY customer_id) AS c"), + same("COUNT DISTINCT", "COUNT(DISTINCT city) OVER (PARTITION BY country) AS c"), + same("FIRST_VALUE", "FIRST_VALUE(amount) OVER (PARTITION BY product ORDER BY ts ASC) AS f"), + same("LAST_VALUE", "LAST_VALUE(amount) OVER (PARTITION BY product ORDER BY ts ASC) AS l"), + same("ARRAY_AGG", "ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC) AS t"), + same("STDDEV", "STDDEV(salary) OVER (PARTITION BY department) AS sd"), + same("STDDEV_SAMP", "STDDEV_SAMP(salary) OVER (PARTITION BY department) AS sd"), + same("STDDEV_POP", "STDDEV_POP(salary) OVER (PARTITION BY department) AS sd"), + same("VARIANCE", "VARIANCE(salary) OVER (PARTITION BY department) AS v"), + same("VAR_SAMP", "VAR_SAMP(salary) OVER (PARTITION BY department) AS v"), + same("VAR_POP", "VAR_POP(salary) OVER (PARTITION BY department) AS v"), + same("ROW_NUMBER", "ROW_NUMBER() OVER (PARTITION BY department ORDER BY salary DESC) AS rn"), + same("RANK", "RANK() OVER (PARTITION BY department ORDER BY salary DESC) AS rk"), + same("DENSE_RANK", "DENSE_RANK() OVER (PARTITION BY department ORDER BY salary DESC) AS dr"), + same("ranking without PARTITION BY", "ROW_NUMBER() OVER (ORDER BY salary DESC) AS rn"), + // ranking windows DO carry their inline LIMIT through the rendering (RankingWindow overrides + // emitsLimitInOver); the ARRAY_AGG case below shows the other half of that decision. + same( + "RANK inline LIMIT", + "RANK() OVER (PARTITION BY department ORDER BY salary DESC LIMIT 3) AS rk" + ), + // all five percentile spellings converge on one canonical rendering. This is the PR's headline + // finding, and it is only a finding because the expected string is pinned. + ( + "PERCENTILE_CONT OVER", + "PERCENTILE_CONT(0.95) OVER (PARTITION BY service ORDER BY ms) AS p", + "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) OVER (PARTITION BY service) AS p" + ), + ( + "PERCENTILE_DISC OVER", + "PERCENTILE_DISC(0.95) OVER (PARTITION BY service ORDER BY ms) AS p", + "PERCENTILE_DISC(0.95) WITHIN GROUP (ORDER BY ms) OVER (PARTITION BY service) AS p" + ), + same("PERCENTILE WITHIN GROUP", "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) AS p"), + same( + "PERCENTILE WITHIN + OVER", + "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) OVER (PARTITION BY service) AS p" + ), + ( + "PERCENTILE shorthand", + "PERCENTILE_CONT(ms, 0.95) AS p", + "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) AS p" + ), + ( + "PERCENTILE shorthand + OVER", + "PERCENTILE_CONT(ms, 0.95) OVER (PARTITION BY service) AS p", + "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY ms) OVER (PARTITION BY service) AS p" + ), + // the [0,1] bound is inclusive; the rejections below only probe it from outside + ( + "PERCENTILE p = 0", + "PERCENTILE_CONT(0) WITHIN GROUP (ORDER BY ms) AS p", + "PERCENTILE_CONT(0.0) WITHIN GROUP (ORDER BY ms) AS p" + ), + ( + "PERCENTILE p = 1", + "PERCENTILE_CONT(1) WITHIN GROUP (ORDER BY ms) AS p", + "PERCENTILE_CONT(1.0) WITHIN GROUP (ORDER BY ms) AS p" + ) + ) + } + + documented.foreach { case (label, written, expected) => + it should s"render the documented window form as documented: $label" in { + val statement = Parser(select + written + from) match { + case Right(st) => st + case Left(err) => fail(s"documented form failed to parse: $written -> $err") + } + statement.sql shouldBe (select + expected + from) + withClue(s"the rendering does not parse back: ${statement.sql}\n") { + Parser(statement.sql).isRight shouldBe true } } } - /** Rejections the documentation states explicitly; they must stay rejections. */ - private val rejected: Seq[(String, String)] = Seq( - // ANSI: a ranking with no order is not a ranking. Rejected at parse time on purpose, rather - // than parsing and breaking at execution. - "ranking window without ORDER BY" -> - "SELECT name, ROW_NUMBER() OVER (PARTITION BY department) AS rn FROM employees", - "percentile fraction above 1" -> - "SELECT PERCENTILE_CONT(1.5) WITHIN GROUP (ORDER BY ms) AS p FROM logs", - "percentile fraction below 0" -> - "SELECT PERCENTILE_CONT(-0.5) WITHIN GROUP (ORDER BY ms) AS p FROM logs" - ) + /** ⚠️ Known loss, pinned deliberately so the day it is fixed this test says so. + * + * `ARRAY_AGG`'s inline `LIMIT` is parsed and kept on the AST, but `emitsLimitInOver` is false + * for everything except ranking windows, so the rendering drops it. Any consumer that + * round-trips SQL through the AST — the REPL, JOIN reconstruction — loses the bound silently. + * See SoftClient4ES issue for the render/parse asymmetry; `dql_statements.md` documents this + * `LIMIT` as meaningful. + */ + it should "drop ARRAY_AGG's inline LIMIT when rendering (known asymmetry)" in { + val written = "ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC LIMIT 10) AS t" + val Right(statement) = Parser(select + written + from) + statement.sql shouldBe + (select + "ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC) AS t" + from) + } + + /** Rejections the documentation states explicitly. Each asserts WHY it was rejected: a negative + * that only checks `isLeft` passes just as happily when the construct stops being recognised at + * all, which is the opposite of what it claims to prove. + */ + it should "reject a ranking window with no ORDER BY, and accept the same statement with one" in { + val withoutOrder = s"${select}ROW_NUMBER() OVER (PARTITION BY department) AS rn$from" + val withOrder = + s"${select}ROW_NUMBER() OVER (PARTITION BY department ORDER BY salary DESC) AS rn$from" + // paired on purpose: the rejection message here is a generic one, so on its own it cannot tell + // "the ANSI rule fired" from "ROW_NUMBER is no longer a window function". The pair can. + Parser(withoutOrder).isRight shouldBe false + Parser(withOrder).isRight shouldBe true + } - rejected.foreach { case (label, sql) => - it should s"reject: $label" in { - Parser(sql).isRight shouldBe false + Seq("above 1" -> "1.5", "below 0" -> "-0.5").foreach { case (label, p) => + it should s"reject a percentile fraction $label, naming the bound" in { + Parser(s"${select}PERCENTILE_CONT($p) WITHIN GROUP (ORDER BY ms) AS p$from") match { + case Right(st) => fail(s"expected rejection, parsed as: ${st.sql}") + case Left(err) => err.toString should include("[0,1]") + } } } } From e0339d0892205b89dc15d19241141cd2d4736d02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Wed, 26 Aug 2026 21:17:51 +0200 Subject: [PATCH 3/3] docs(sql): cite #247 in the pinned ARRAY_AGG LIMIT test 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. --- .../elastic/sql/parser/WindowFunctionCoverageSpec.scala | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala index 41852183..17332f13 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/WindowFunctionCoverageSpec.scala @@ -111,9 +111,10 @@ class WindowFunctionCoverageSpec extends AnyFlatSpec with Matchers { * * `ARRAY_AGG`'s inline `LIMIT` is parsed and kept on the AST, but `emitsLimitInOver` is false * for everything except ranking windows, so the rendering drops it. Any consumer that - * round-trips SQL through the AST — the REPL, JOIN reconstruction — loses the bound silently. - * See SoftClient4ES issue for the render/parse asymmetry; `dql_statements.md` documents this - * `LIMIT` as meaningful. + * round-trips SQL through the AST — the REPL, JOIN reconstruction — loses the bound. Worse, + * `ArrayAgg.update` then falls back to `request.limit`, so the statement's own LIMIT is + * substituted for the inline one — a different answer, not an error. See issue #247. Pinned to + * today's behaviour on purpose: fixing #247 makes this test fail, which is the point. */ it should "drop ARRAY_AGG's inline LIMIT when rendering (known asymmetry)" in { val written = "ARRAY_AGG(tag) OVER (PARTITION BY product ORDER BY ts ASC LIMIT 10) AS t"