fix(sql): close exactly the parentheses a function call opened (#220)

bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala

@@ -1444,6 +1444,11 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers {
         |        }
         |      },
         |      "aggs": {
+        |        "ct": {
+        |          "value_count": {
+        |            "field": "identifier2"
+        |          }
+        |        },
         |        "lastSeen": {
         |          "max": {
         |            "field": "createdAt",
@@ -1452,11 +1457,6 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers {
         |              "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); (param1 == null) ? null : ZonedDateTime.parse(param1, DateTimeFormatter.ofPattern(\"yyyy-MM-dd HH:mm:ss.SSS XXX\")).truncatedTo(ChronoUnit.MINUTES).get(ChronoField.YEAR)"
         |            }
         |          }
-        |        },
-        |        "ct": {
-        |          "value_count": {
-        |            "field": "identifier2"
-        |          }
         |        }
         |      }
         |    }

documentation/sql/functions_type_conversion.md

@@ -16,21 +16,27 @@ CONVERT(expr, TYPE)
 **Inputs:**
 - `expr` - Expression to convert
 - `TYPE` - Target data type:
-  - `VARCHAR` / `STRING` / `TEXT`
+  - `VARCHAR` / `STRING` / `CHAR`
   - `INT` / `INTEGER` / `BIGINT` / `SMALLINT` / `TINYINT`
   - `DOUBLE` / `FLOAT` / `REAL`
-  - `DECIMAL(p, s)` / `NUMERIC(p, s)`
-  - `BOOLEAN` / `BOOL`
+  - `BOOLEAN`
   - `DATE`
   - `TIMESTAMP` / `DATETIME`
   - `TIME`
 
+> `DECIMAL` / `NUMERIC` are **not** cast targets (see
+> [known limitations](known_limitations.md)) — use `DOUBLE` and round explicitly. `TEXT`, `KEYWORD`
+> and `BOOL` are column types in `CREATE TABLE`, not cast targets; write `VARCHAR` and `BOOLEAN`.
+
 **Output:**
 - Value converted to target `TYPE`
 
 **Behavior:**
 - Throws error if conversion fails
 - Use `TRY_CAST` for safe conversion
+- A cast cannot wrap an **aggregate result** — cast the aggregate's input instead
+  (`MAX(salary::BIGINT)`, not `MAX(salary)::BIGINT`). See
+  [Type Conversion](type_conversion.md#restriction-cast-the-input-of-an-aggregate-not-its-result).
 
 **Examples:**
 

documentation/sql/type_conversion.md

@@ -16,7 +16,7 @@ Converts a value to a specified SQL type. Fails if the conversion is invalid.
 
 **Example:**
 ```sql
-SELECT CAST('2025-09-11' AS DATE) AS d;
+SELECT CAST('2025-09-11' AS DATE) AS d FROM logs;
 -- Result: 2025-09-11
 ```
 
@@ -36,7 +36,7 @@ Attempts to convert a value to a specified SQL type. Returns `NULL` if the conve
 
 **Example:**
 ```sql
-SELECT TRY_CAST('invalid-date' AS DATE) AS d;
+SELECT TRY_CAST('invalid-date' AS DATE) AS d FROM logs;
 -- Result: NULL
 ```
 
@@ -56,7 +56,7 @@ Shorthand operator for casting. Equivalent to `CAST(value AS type)`.
 
 **Example:**
 ```sql
-SELECT '2025-09-11'::DATE AS d, '125'::BIGINT AS b;
+SELECT '2025-09-11'::DATE AS d, '125'::BIGINT AS b FROM logs;
 -- Result: 2025-09-11, 125
 ```
 
@@ -69,4 +69,36 @@ SELECT '2025-09-11'::DATE AS d, '125'::BIGINT AS b;
 - `::` is syntactic sugar, easier to read in queries.  
 - Type inference relies on `baseType`, and explicit `CAST`/`TRY_CAST`/`::` updates the type context for following functions.  
 
+---
+
+## Restriction: cast the input of an aggregate, not its result
+
+A cast cannot be applied to the **result** of an aggregate function. An aggregate has to be the
+first function in a column's chain, and a cast wrapping it would sit ahead of it:
+
+```sql
+-- Rejected: "Aggregation function must be the first function in the chain"
+SELECT MAX(salary)::BIGINT AS m FROM emp GROUP BY dept;
+SELECT CAST(MAX(salary) AS BIGINT) AS m FROM emp GROUP BY dept;
+```
+
+All four spellings behave the same way — `::`, `CAST`, `TRY_CAST` and `CONVERT`.
+
+Cast the aggregate's **input** instead. This is equivalent for every aggregate whose result type
+follows its argument (`MIN`, `MAX`, `SUM`, `AVG`, the `STDDEV` / `VARIANCE` family, the percentiles):
+
+```sql
+-- Both supported
+SELECT MAX(salary::BIGINT) AS m FROM emp GROUP BY dept;
+SELECT MAX(CAST(salary AS BIGINT)) AS m FROM emp GROUP BY dept;
+```
+
+Casting is unrestricted everywhere an aggregate is not involved — the SELECT list, `WHERE`,
+`ORDER BY`, and inside another function:
+
+```sql
+SELECT YEAR(createdAt)::VARCHAR AS y FROM logs;
+SELECT id FROM logs WHERE tries::INT > 3 ORDER BY tries::INT;
+```
+
 [Back to index](README.md)

es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala

@@ -1444,6 +1444,11 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers {
       |        }
       |      },
       |      "aggs": {
+      |        "ct": {
+      |          "value_count": {
+      |            "field": "identifier2"
+      |          }
+      |        },
       |        "lastSeen": {
       |          "max": {
       |            "field": "createdAt",
@@ -1452,11 +1457,6 @@ class SQLQuerySpec extends AnyFlatSpec with Matchers {
       |              "source": "def param1 = (doc['createdAt'].size() == 0 ? null : doc['createdAt'].value); (param1 == null) ? null : ZonedDateTime.parse(param1, DateTimeFormatter.ofPattern(\"yyyy-MM-dd HH:mm:ss.SSS XXX\")).truncatedTo(ChronoUnit.MINUTES).get(ChronoField.YEAR)"
       |            }
       |          }
-      |        },
-      |        "ct": {
-      |          "value_count": {
-      |            "field": "identifier2"
-      |          }
       |        }
       |      }
       |    }

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

@@ -249,20 +249,19 @@ object Parser
 
   /** The `SCRIPT AS ( … )` body, delimited by scanning its own balanced parentheses.
     *
-    * `scriptValue` cannot be relied on to stop at the wrapper's closing paren: the generic
-    * `identifierWithFunction` closes with `rep1(end)` — one or MORE — so a call to a name-only
-    * extractor (`YEAR(x)`, `MONTH(x)`, `QUARTER(x)`, …) consumed the `)` belonging to the wrapper.
-    * `script` then failed, and a column definition fell through to `optionalMultiFields`, which is
-    * why `age INT SCRIPT AS (YEAR(CURRENT_DATE) - YEAR(birthdate))` — the documented example —
-    * reported `')' expected but 'S' found`. `SELECT YEAR(x)` was never affected: nothing enclosed
-    * it.
+    * Historically `scriptValue` could not be relied on to stop at the wrapper's closing paren: the
+    * generic `identifierWithFunction` closed with `rep1(end)` — one or MORE — so a call to a
+    * name-only extractor (`YEAR(x)`, `MONTH(x)`, `QUARTER(x)`, …) consumed the `)` belonging to the
+    * wrapper. `script` then failed, and a column definition fell through to `optionalMultiFields`,
+    * which is why `age INT SCRIPT AS (YEAR(CURRENT_DATE) - YEAR(birthdate))` — the documented
+    * example — reported `')' expected but 'S' found`.
     *
-    * Balancing that shared production is NOT the fix, and three attempts proved it: `rep1sep(
-    * sql_function, start)` is ambiguous, and the greed of `rep1(end)` is what selects the correct
-    * parse for a nested call like `MAX(YEAR(DATE_TRUNC(DATETIME_PARSE(…), MINUTE)))`. Bounding the
-    * close count, or giving the extractors their own balanced production, silently drops `Year`
-    * from `identifier.functions` — while `.sql` still renders it, so only the emitted painless
-    * script shows the loss. Deciding the boundary here instead leaves every other parse untouched.
+    * `identifierWithFunction` has since been made balanced (issue #220), so it no longer overruns.
+    * This scanner is kept because it is the stronger guarantee: the body's extent is decided here
+    * rather than inferred from whichever alternative happens to win inside it, which is what makes
+    * the diagnostics below — unbalanced parentheses, a missing `(`, an unparseable body — possible
+    * at all. Deleting it would put the wrapper's boundary back at the mercy of the expression
+    * grammar.
     */
   private def scriptBody: Parser[String] = new Parser[String] {
     def apply(in: Input): ParseResult[String] = {
@@ -1663,21 +1662,36 @@ trait Parser
     geoFunctionWithIdentifier) >> cast
 
   def identifierWithFunction: PackratParser[Identifier] =
-    (rep1sep(
+    ((rep1sep(
       sql_function,
       start
-    ) ~ start.? ~ (identifierWithTransformation | identifierWithIntervalFunction | identifier).? ~ rep1(
-      end
-    ) ^^ { case f ~ _ ~ i ~ _ =>
-      i match {
-        case None =>
-          f.lastOption match {
-            case Some(fi: FunctionWithIdentifier) =>
-              fi.identifier.withFunctions(f ++ fi.identifier.functions)
-            case _ => Identifier(f)
+    ) ~ start.? ~ (identifierWithTransformation | identifierWithIntervalFunction | identifier).?) >> {
+      case f ~ s ~ i =>
+        // Close exactly the parentheses this production opened — one per `rep1sep` separator, plus
+        // the optional one introducing the innermost identifier. It used to close with `rep1(end)`
+        // — one or MORE — which made a call to a name-only extractor (`YEAR(x)`, `MONTH(x)`, …)
+        // swallow the `)` of whatever enclosed it, so `ABS(YEAR(x))` reported `')' expected but 'F'
+        // found` at the FROM (issue #220), and `SCRIPT AS (YEAR(x))` blamed the SCRIPT keyword
+        // (issue #219). Functions that consume their own parentheses (DATE_TRUNC, DATE_DIFF,
+        // WEEKDAY, …) were never affected, which is why the failure looked arbitrary.
+        val opened = f.size - 1 + (if (s.isDefined) 1 else 0)
+        if (opened < 1)
+          // No parenthesis was opened, so this is a bare function name rather than a call — the
+          // `rep1` this replaced rejected it too, and accepting it would let `SELECT MAX FROM t`
+          // parse as a function applied to nothing.
+          failure("function call expected")
+        else
+          repN(opened, end) ^^ { _ =>
+            i match {
+              case None =>
+                f.lastOption match {
+                  case Some(fi: FunctionWithIdentifier) =>
+                    fi.identifier.withFunctions(f ++ fi.identifier.functions)
+                  case _ => Identifier(f)
+                }
+              case Some(id) => id.withFunctions(f ++ id.functions)
+            }
           }
-        case Some(id) => id.withFunctions(f ++ id.functions)
-      }
     }) >> cast
 
   private val regexAlias =

sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala

@@ -80,13 +80,24 @@ case class Field(
     identifier.windows match {
       case Some(th) =>
         val windowFunction = th.update(request)
-        val identifier = windowFunction.identifier
-        identifier.functions match {
-          case _ :: tail =>
-            this.copy(identifier = identifier.withFunctions(functions = windowFunction +: tail))
-          case _ =>
-            this.copy(identifier = identifier.withFunctions(functions = List(windowFunction)))
+        val windowed = windowFunction.identifier
+        // A field's chain is `<applied after the window> :: window :: <applied before it>`, and
+        // both ends were being lost. The trailing end went first: this dropped the HEAD of the
+        // WINDOW's identifier — a leftover from when the trimmed list was the field's own, whose
+        // head IS the window function. Once the source became the window's identifier the head
+        // stopped being the window and became the innermost transform, so
+        // `MAX(YEAR(DATE_TRUNC(x, MINUTE)))` silently lost `YEAR` and emitted a painless script
+        // without `.get(ChronoField.YEAR)`. The leading end was lost too, because rebuilding from
+        // the window's identifier alone cannot express anything wrapping the window: a postfix
+        // cast parses to `CastOperator :: MaxAgg :: …`, and `MAX(salary)::STRING` came back
+        // un-cast, the round trip quietly shorter than the statement.
+        val outer = identifier.updateFunctions(request).takeWhile {
+          case _: WindowFunction => false
+          case _                 => true
         }
+        this.copy(identifier =
+          windowed.withFunctions(functions = outer ++ (windowFunction +: windowed.functions))
+        )
       case None => this.copy(identifier = identifier.update(request))
     }
   }