Fix #284 and #285 — quoted operand may head arithmetic; a dangling dot no longer eats the next word

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)
 

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 |

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 |

sql/src/main/scala/app/softnetwork/elastic/sql/parser/Parser.scala

@@ -1827,25 +1827,52 @@ 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))
 
   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.

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 |

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 |

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