From 5df9b1b317762cf0195bb33b9b0b2ca437124661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Manciot?= Date: Sun, 6 Sep 2026 01:28:02 +0200 Subject: [PATCH] fix(sql): quoted operand may head an arithmetic expression; a dangling dot no longer eats the next word MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #284 Closes #285 Both defects were found implementing story 21.1 (#252 part 1) and were pinned as accepted limitations rather than fixed. They should not have shipped that way: a PR does not bring new issues with it. #284 — a quoted lexeme at the HEAD of an arithmetic expression was consumed alone and the operator left unconsumed, so `SELECT `amount` + 1` failed with `end of input expected` while `SELECT (`amount` + 1)` and `SELECT MAX(`amount` + 1)` worked. The four clause-level productions list `quotedIdentifier` ahead of `identifierWithArithmeticExpression` and `|` commits to the first SUCCEEDING alternative. That ordering cannot be reversed — it is what makes `SELECT "category"` a column rather than a string literal (21.1 AD-3) — so the fix is a one-token lookahead: `quotedIdentifierUnlessArithmetic` declines to match only when an operator is genuinely next. `not(...)` consumes nothing, so every other input reaches `quotedIdentifier` unchanged. Deliberately NOT applied to `quotedIdentifier` itself: in operand position, inside `identifierWithIntervalFunction`, matching the bare name is exactly what lets `arithmeticExpressionLevel1`'s `rep` pick up the rest of the expression. Guarding it there would push the operand down to `identifierWithValue` and turn it back into a string — the AD-13 corruption in reverse. #285 — `nameTail` was `rep("." ~> part)`, and RegexParsers skips whitespace before every terminal, so a name ending in a dot swallowed the next word: `ORDER BY b. DESC` parsed as `ORDER BY "b.DESC" ASC` and the sort direction vanished SILENTLY. The separator now belongs to `nameTailPartRegex`, which matches the dot together with its part (bare or quoted), so the two must be adjacent. `ORDER BY b. DESC` is now a loud rejection instead of a wrong answer. This reverts the `SELECT a . b` widening story 21.1 had pinned — the adjacency rule is what closes the silent defect, and the spacing is not a spelling anything emits. Residual, accepted and pinned: whitespace BEFORE the dot is still skipped, so `SELECT a .b` reads as `a.b`; nothing is lost and no clause is mis-parsed. Verified: sql 655 and core 868 green on BOTH 2.13.16 and 2.12.20, bridge template 129, macrosTests 19, scalafmtCheckAll and headerCheck clean. Two test rows corrected while writing this — `SELECT `category`, `amount` + 1 ... GROUP BY `category`` is rejected by validate() for the bare spelling too (not a quoting limit, now pinned as a pair), and the render of an arithmetic expression carries the canonical quoting (`"amount" + 1`). Co-Authored-By: Claude Opus 5 (1M context) --- documentation/sql/known_limitations.md | 10 +- .../elastic/sql/parser/GroupByParser.scala | 3 +- .../elastic/sql/parser/OrderByParser.scala | 3 +- .../elastic/sql/parser/Parser.scala | 70 ++++++++-- .../elastic/sql/parser/SelectParser.scala | 4 +- .../elastic/sql/parser/WhereParser.scala | 3 +- .../sql/parser/QuotedIdentifierSpec.scala | 120 +++++++++++++----- 7 files changed, 161 insertions(+), 52 deletions(-) diff --git a/documentation/sql/known_limitations.md b/documentation/sql/known_limitations.md index ae679377..5f5bb574 100644 --- a/documentation/sql/known_limitations.md +++ b/documentation/sql/known_limitations.md @@ -85,7 +85,7 @@ WHERE department_id IN (SELECT id FROM departments WHERE region = 'EU'); The parser rejects this — `IN` accepts only literal value lists today, not a nested `SELECT`. Rewrite it as an explicit JOIN (fully supported), or wait for the next release where the subquery form lands as-is. -## Quoted identifiers — three residual limits +## Quoted identifiers — two residual limits Quoted column names and aliases work in both spellings — see [Quoted identifiers](dql_statements.md#quoted-identifiers). Three things they do **not** cover yet: @@ -102,10 +102,10 @@ Quoted column names and aliases work in both spellings — see Elasticsearch field whose own name contains a dot. Quoting makes it *look* as though there should be; there is not. -- **An arithmetic expression cannot START with a quoted operand, unparenthesised.** - `` SELECT `amount` + 1 `` and `SELECT "amount" + 1` are rejected, while `SELECT amount + 1`, - `` SELECT (`amount` + 1) `` and `` SELECT MAX(`amount` + 1) `` all work. Wrap the expression in - parentheses — which is what every BI tool emits for a calculation anyway. +- **A dot and the name part after it must be adjacent.** `SELECT a.b` is a qualified name; + `SELECT a . b` is rejected, and so is a name left with a trailing dot (`ORDER BY b. DESC`). This + is deliberate: when the dot was allowed to float, `ORDER BY b. DESC` silently parsed as a column + named `b.DESC` sorted *ascending*. ## Coming in the upcoming release (Quarter 1 2027) diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/GroupByParser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/GroupByParser.scala index 383d79a5..1de5b121 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/GroupByParser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/GroupByParser.scala @@ -23,7 +23,8 @@ trait GroupByParser { self: Parser with WhereParser => def bucketWithFunction: PackratParser[Identifier] = - quotedIdentifier | + // #284 - see quotedIdentifierUnlessArithmetic. + quotedIdentifierUnlessArithmetic | identifierWithArithmeticExpression | identifierWithTransformation | identifierWithWindowFunction | diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/OrderByParser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/OrderByParser.scala index d03db314..7305c93e 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/OrderByParser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/OrderByParser.scala @@ -45,7 +45,8 @@ trait OrderByParser { """\b(?!(?i)limit\b)[a-zA-Z_][a-zA-Z0-9_]*""".r ^^ (f => f) def fieldWithFunction: PackratParser[Identifier] = - quotedIdentifier | + // #284 - see quotedIdentifierUnlessArithmetic. + quotedIdentifierUnlessArithmetic | identifierWithArithmeticExpression | identifierWithTransformation | identifierWithWindowFunction | diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala index 1aadef32..9200f0a2 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala @@ -1827,13 +1827,15 @@ trait Parser */ private val bareNextPartStr = s"$barePartChars+" - /** Both regexes are compiled ONCE, as `val`s, exactly like `identifierRegex` above. - * `bareFirstPartStr` interpolates the reserved-keyword alternation, and `String.r` is - * `Pattern.compile` -- calling it inside a `def` recompiles it on every construction of the - * hottest production in the grammar, now also once per `rep` iteration. + /** Compiled ONCE, as a `val`, exactly like `identifierRegex` above. `bareFirstPartStr` + * interpolates the reserved-keyword alternation, and `String.r` is `Pattern.compile` -- calling + * it inside a `def` recompiles it on every construction of the hottest production in the + * grammar. + * + * `bareNextPartStr` has no regex of its own: it is interpolated into `nameTailPartStr`, which + * owns the separator so the dot and the part cannot be split by whitespace (#285). */ private val bareFirstPartRegex: Regex = bareFirstPartStr.r - private val bareNextPartRegex: Regex = bareNextPartStr.r private def quotedPart: PackratParser[(String, Boolean)] = quotedNameRegex ^^ (lexeme => (unquoteName(lexeme), true)) @@ -1841,11 +1843,36 @@ trait Parser private def bareFirstPart: PackratParser[(String, Boolean)] = bareFirstPartRegex ^^ (n => (n, false)) - private def bareNextPart: PackratParser[(String, Boolean)] = - bareNextPartRegex ^^ (n => (n, false)) + /** One dot-separated tail element, **separator included**, matched as a SINGLE regex so the dot + * and the part that follows it must be ADJACENT. + * + * `RegexParsers` skips whitespace before every terminal, so the earlier `rep("." ~> part)` + * spelling let a name ending in a dot swallow the next word across whitespace (#285): `ORDER BY + * b. DESC` parsed as `ORDER BY "b.DESC" ASC` and the sort direction vanished **silently**. + * Owning the dot closes that -- after `b` the tail cannot match `. DESC`, so `b` is the name and + * `DESC` is the direction -- and it also restores the pre-21.1 rejection of `SELECT a . b`, + * which the part-split had widened into an acceptance. + * + * Residual, deliberately accepted: whitespace BEFORE the dot is still skipped by the enclosing + * `rep`, so `SELECT a .b` reads as `a.b`. That is a tolerance of an odd spelling, not a silent + * reading change -- no clause is mis-parsed and nothing is lost. + */ + private val nameTailPartStr = + s"""\\.(?:$doubleQuotedNameStr|$backQuotedNameStr|$bareNextPartStr)""" + + /** Compiled once, for the same reason as `bareFirstPartRegex`. */ + private val nameTailPartRegex: Regex = nameTailPartStr.r + + private def nameTailPart: PackratParser[(String, Boolean)] = + nameTailPartRegex ^^ { lexeme => + val part = lexeme.substring(1) // drop the leading dot, which this regex owns + part.charAt(0) match { + case '"' | '`' => (unquoteName(part), true) + case _ => (part, false) + } + } - private def nameTail: PackratParser[List[(String, Boolean)]] = - rep("." ~> (quotedPart | bareNextPart)) + private def nameTail: PackratParser[List[(String, Boolean)]] = rep(nameTailPart) private def joinNameParts(parts: List[(String, Boolean)]): (String, Boolean) = (parts.map(_._1).mkString("."), parts.exists(_._2)) @@ -1887,6 +1914,31 @@ trait Parser GenericIdentifier(nq._1, None, d.isDefined, quoted = nq._2) }) >> cast + /** `quotedIdentifier`, but it declines to match when an arithmetic operator follows it (#284). + * + * The four clause-level productions -- `SelectParser.field`, `GroupByParser.bucketWithFunction`, + * `OrderByParser.fieldWithFunction` and `WhereParser.any_identifier` -- all list + * `quotedIdentifier` AHEAD of `identifierWithArithmeticExpression`, and `|` commits to the first + * SUCCEEDING alternative. So a quoted lexeme at the HEAD of an arithmetic expression was + * consumed alone and the operator was left unconsumed: ``SELECT `amount` + 1`` failed with `end + * of input expected`, while ``SELECT (`amount` + 1)`` and ``SELECT MAX(`amount` + 1)`` both + * worked. + * + * That ordering cannot simply be reversed. It is what makes `SELECT "category"` a COLUMN rather + * than a string literal, because a double-quoted lexeme also matches `TypeParser.literal` (story + * 21.1 AD-3). A one-token LOOKAHEAD is the narrow fix: it changes only which alternative wins + * when an operator is genuinely next, and `not(...)` consumes nothing, so every other input + * reaches `quotedIdentifier` exactly as before. + * + * Deliberately NOT applied to `quotedIdentifier` itself. In OPERAND position -- inside + * `identifierWithIntervalFunction`, reached from `factor` -- `quotedIdentifier` matching the + * bare name is precisely what lets `arithmeticExpressionLevel1`'s `rep` pick up the rest of the + * expression. Guarding it there would push the operand down to `identifierWithValue` and turn it + * back into a string, which is the AD-13 corruption in reverse. + */ + def quotedIdentifierUnlessArithmetic: PackratParser[Identifier] = + quotedIdentifier <~ not(add | subtract | multiply | divide | modulo) + /** THE identifier production. Quoting is folded in here rather than sprinkled over the ~35 sites * that end in `| identifier` -- the four-alternative operand idiom alone occurs 21 times -- so a * production added later inherits it instead of having to remember it. diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/SelectParser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/SelectParser.scala index ef9bde15..baece19d 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/SelectParser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/SelectParser.scala @@ -22,7 +22,9 @@ trait SelectParser { self: Parser with WhereParser => def field: PackratParser[Field] = - (quotedIdentifier | + // #284: decline the quoted lexeme when an arithmetic operator follows, so + // `SELECT `amount` + 1` reaches identifierWithArithmeticExpression below. + (quotedIdentifierUnlessArithmetic | identifierWithArithmeticExpression | identifierWithTransformation | identifierWithWindowFunction | diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala index 5b87f61c..f796d755 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/parser/WhereParser.scala @@ -95,7 +95,8 @@ trait WhereParser { def diff: PackratParser[ComparisonOperator] = DIFF.sql ^^ (_ => DIFF) private def any_identifier: PackratParser[Identifier] = - quotedIdentifier | + // #284 - see quotedIdentifierUnlessArithmetic. + quotedIdentifierUnlessArithmetic | identifierWithArithmeticExpression | identifierWithTransformation | identifierWithWindowFunction | diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedIdentifierSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedIdentifierSpec.scala index 4af8fe7c..f079a023 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedIdentifierSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/QuotedIdentifierSpec.scala @@ -263,32 +263,68 @@ class QuotedIdentifierSpec extends AnyFlatSpec with Matchers { firstFieldName("SELECT SUM(`bi_events`.`amount`) AS s FROM bi_events") shouldBe "amount" } - /** MEASURED LIMITATION, both spellings, unchanged by this story. + /** #284 — FIXED. An unparenthesised arithmetic expression may be headed by a quoted operand. * - * `quotedIdentifier` is the FIRST alternative of `SelectParser.field` and of - * `WhereParser.any_identifier`, and `|` commits to the first SUCCEEDING alternative, so a quoted - * lexeme at the head of an arithmetic expression is consumed alone and the operator is left - * unconsumed. That ordering is exactly what makes `SELECT "category"` a column rather than a - * string, so it cannot be relaxed here. + * It used to be rejected with `end of input expected`, in both spellings: the four clause-level + * productions list `quotedIdentifier` AHEAD of `identifierWithArithmeticExpression` and `|` + * commits to the first SUCCEEDING alternative, so the quoted lexeme was consumed alone and the + * operator was left unconsumed. That ordering could not simply be reversed — it is what makes + * `SELECT "category"` a column rather than a string (AD-3) — so the fix is the one-token + * LOOKAHEAD in `quotedIdentifierUnlessArithmetic`, which declines to match only when an operator + * is genuinely next. * - * `SELECT "amount" + 1` is rejected on the unmodified tree for this same reason (measured, with - * `end of input expected` — NOT the type error an earlier reading of the code predicted), so - * this is a pre-existing limitation the backtick spelling inherits, not a regression. The - * parenthesised form — which is what every BI tool emits for a calculation — works. + * Asserting `isRight` alone would be a certification, not a test: the whole point is that the + * operand is the COLUMN, so the AST is asserted too. */ - it should "reject an UNPARENTHESISED arithmetic expression headed by a quoted operand" in { - rejected("SELECT `amount` + 1 AS a FROM t") - rejected("SELECT \"amount\" + 1 AS a FROM t") - rejected("SELECT id FROM t WHERE `amount` + 1 > 5") - // The bare spelling has no such limitation — `quotedIdentifier` cannot match it at all. + it should "accept an UNPARENTHESISED arithmetic expression headed by a quoted operand (#284)" in { + parses("SELECT `amount` + 1 AS a FROM t") + parses("SELECT \"amount\" + 1 AS a FROM t") + parses("SELECT id FROM t WHERE `amount` + 1 > 5") + parses("SELECT `amount` - 1 AS a FROM t") + parses("SELECT `amount` * 2 AS a FROM t") + parses("SELECT `amount` / 2 AS a FROM t") + parses("SELECT `amount` % 2 AS a FROM t") + parses("SELECT `a`.`amount` + 1 AS x FROM t a") + parses("SELECT `amount` + `qty` AS a FROM t") + parses("SELECT `category`, SUM(`amount` + 1) AS a FROM t GROUP BY `category`") + parses("SELECT id FROM t ORDER BY `amount` + 1 DESC") + // NOT a quoting limit: a bare `amount + 1` beside a GROUP BY is rejected identically, by + // `validate()` and not by the grammar. Pinned as a pair so the two reasons stay distinguishable. + rejected("SELECT `category`, `amount` + 1 AS a FROM t GROUP BY `category`") + rejected("SELECT category, amount + 1 AS a FROM t GROUP BY category") + + // The operand is the COLUMN, not the string — the reading the lookahead has to preserve. The + // render carries the canonical quoting, so the arithmetic reads `"amount" + 1`. + Parser("SELECT `amount` + 1 AS a FROM t").toOption.get.sql should include("\"amount\" + 1") + Parser("SELECT \"amount\" + 1 AS a FROM t").toOption.get.sql should include("\"amount\" + 1") + single("SELECT `amount` + 1 AS a FROM t").select.fields.head.identifier.sql should + include("\"amount\"") + + // The bare spelling never had the limitation. parses("SELECT amount + 1 AS a FROM t") parses("SELECT id FROM t WHERE amount + 1 > 5") - // …and the parenthesised and nested forms work for the quoted spellings. + // …and the parenthesised and nested forms keep working. parses("SELECT (`amount` + 1) AS a FROM t") parses("SELECT (\"amount\" + 1) AS a FROM t") parses("SELECT MAX(`amount` + 1) AS a FROM t") } + /** The guard is a LOOKAHEAD, so it must consume nothing. If `not(...)` ever became a consuming + * parser, or the guard were applied to `quotedIdentifier` itself rather than to the clause-level + * alternative, these would break — each is a quoted operand with NO operator after it, in each + * of the four guarded productions. + */ + it should "leave a quoted identifier with no operator after it completely unaffected" in { + firstFieldName("SELECT `category` FROM t") shouldBe "category" + single("SELECT `category`, COUNT(id) AS n FROM t GROUP BY `category`").sql should + include("GROUP BY \"category\"") + single("SELECT id FROM t ORDER BY `event_ts` DESC").sql should include("\"event_ts\"") + single("SELECT id FROM t WHERE `category` = 'a'").sql should include("\"category\"") + // A minus sign that is part of the NAME, not an operator, must still be part of the name. + firstFieldName("SELECT `logs-2025` FROM t") shouldBe "logs-2025" + firstFieldName("SELECT logs-2025.03 FROM t") shouldBe "logs-2025.03" + } + "a quoted identifier inside SCRIPT AS" should "parse" in { parses( "ALTER TABLE users ALTER COLUMN age SET SCRIPT AS (DATE_DIFF(`birthdate`, CURRENT_DATE, YEAR))" @@ -439,28 +475,44 @@ class QuotedIdentifierSpec extends AnyFlatSpec with Matchers { s.sql should include("\"e\".\"category\"") } - "whitespace around the dot separator" should "be tolerated" in { - // A widening: rejected on the unmodified tree with `end of input expected`. - firstFieldName("SELECT a . b FROM t") shouldBe "a.b" + /** #285 — FIXED. A name ending in a dot no longer swallows the word that follows it. + * + * `nameTail` used to be `rep("." ~> part)`, and RegexParsers skips whitespace before every + * terminal, so the dot and the next word were joined even when separated. The ORDER BY case was + * SILENT — the direction was absorbed into the name and the sort quietly became ASC. The + * separator now belongs to `nameTailPartRegex`, so the dot and the part must be adjacent. + */ + it should "not let a name ending in a dot swallow the next word (#285)" in { + // The silent one, now LOUD. The tail cannot match `. DESC` across the space, so the dangling + // dot is left unconsumed and `phrase` rejects the statement. A trailing dot is not a name, and + // a rejection is the honest answer — what must never happen again is the old silent + // `ORDER BY "b.DESC" ASC`, where the direction was absorbed into the column name. + rejected("SELECT a FROM t ORDER BY b. DESC") + Parser("SELECT a FROM t ORDER BY b. DESC").swap.toOption.get.msg should not include "b.DESC" + // Same shape in a SELECT list: a loud rejection rather than the column `a.AS`. + rejected("SELECT a. AS x FROM t") + // A well-formed qualified name is unaffected. + single("SELECT a FROM t ORDER BY b.c DESC").sql should include("ORDER BY b.c DESC") + firstFieldName("SELECT a.b FROM t") shouldBe "a.b" + firstFieldName("SELECT logs-2025.03 FROM t") shouldBe "logs-2025.03" + firstFieldName("SELECT a.[0] FROM t") shouldBe "a.[0]" + // Quoted tail parts are joined by the same regex, so they get the same adjacency rule. + single("SELECT `e`.`category` FROM bi_events e").sql should include("\"e\".\"category\"") + rejected("SELECT a FROM t ORDER BY `b`. DESC") } - /** The cost of that widening, measured in review and pinned so it is a decision. - * - * `nameTail` is `rep("." ~> part)` and RegexParsers skips whitespace before every terminal, so a - * name ending in a DOT swallows whatever word follows it — including a keyword. Both inputs - * below are malformed SQL whose old parse was equally nonsense (`identifierRegex` matched the - * trailing dot, giving the field `b.` / `a.`), so this is a change of one broken reading for - * another, not a regression of any valid query. It is pinned because the ORDER BY case is - * SILENT: the direction is absorbed into the name and the sort quietly becomes ASC. + /** Fixing #285 reverted a widening story 21.1 had pinned: `SELECT a . b` parsed as `a.b` while + * the dot was a free-standing terminal. It is a rejection again, as it was before 21.1 — the + * spacing is not a spelling anything emits, and the adjacency rule is what closes the silent + * ORDER BY defect. Pinned so the revert is a decision. * - * Closing it means making the dot and the part one whitespace-free regex, which would also - * revert the widening pinned just above. Recorded, not fixed. + * Residual, deliberately accepted: whitespace BEFORE the dot is still skipped by the enclosing + * `rep`, so `SELECT a .b` reads as `a.b`. That is a tolerance of an odd spelling — nothing is + * lost and no clause is mis-parsed — not a silent reading change. */ - it should "swallow the following word when a name ends in a dot — malformed input, pinned" in { - firstFieldName("SELECT a. AS x FROM t") shouldBe "a.AS" - single("SELECT a FROM t ORDER BY b. DESC").sql should include("ORDER BY b.DESC ASC") - // A well-formed qualified name is unaffected — the direction survives. - single("SELECT a FROM t ORDER BY b.c DESC").sql should include("ORDER BY b.c DESC") + it should "reject a dot separated from its part by whitespace on BOTH sides" in { + rejected("SELECT a . b FROM t") + firstFieldName("SELECT a .b FROM t") shouldBe "a.b" } /** The scanners now treat a backtick as a quote opener, so an ODD backtick outside any quoted run