diff --git a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 73a98468..ad4c7ecd 100644 --- a/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/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" - | } | } | } | } diff --git a/documentation/sql/functions_type_conversion.md b/documentation/sql/functions_type_conversion.md index 602f21fe..64087eda 100644 --- a/documentation/sql/functions_type_conversion.md +++ b/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:** diff --git a/documentation/sql/type_conversion.md b/documentation/sql/type_conversion.md index 53af6718..5fd5b3ca 100644 --- a/documentation/sql/type_conversion.md +++ b/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) diff --git a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala b/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala index 4eec4c00..e8e7aeb2 100644 --- a/es6/bridge/src/test/scala/app/softnetwork/elastic/sql/SQLQuerySpec.scala +++ b/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" - | } | } | } | } 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 267a64f8..7b9a720d 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 @@ -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 = diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala index c0bbb37a..4308ebba 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/query/Select.scala +++ b/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 ` :: window :: `, 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)) } } diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala index d905a316..2dd1365d 100644 --- a/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala @@ -1,6 +1,7 @@ package app.softnetwork.elastic.sql.parser -import app.softnetwork.elastic.sql.query.{AlterTable, CreateTable} +import app.softnetwork.elastic.sql.Identifier +import app.softnetwork.elastic.sql.query.{AlterTable, CreateTable, SingleSearch} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -12,6 +13,12 @@ import org.scalatest.matchers.should.Matchers */ class ScriptFunctionParenSpec extends AnyFlatSpec with Matchers { + private def identifierOf(sql: String): Identifier = + Parser(sql) match { + case Right(s: SingleSearch) => s.select.fields.head.identifier + case other => fail(s"Expected a SingleSearch, got $other") + } + private def scriptOf(sql: String): String = Parser(sql) match { case Right(CreateTable(_, Right(columns), _, _, _, _, _)) => @@ -68,8 +75,6 @@ class ScriptFunctionParenSpec extends AnyFlatSpec with Matchers { "a nested function call" should "still consume its own parentheses" in { // The balance has to hold at every depth, in the context where nothing encloses it … - // `SELECT ABS(YEAR(x))` is deliberately absent: wrapping an extractor in a math function does - // not parse on main either — a separate, pre-existing limitation, not this boundary. Seq( "SELECT YEAR(createdAt) FROM t", "SELECT YEAR(DATE_TRUNC(createdAt, MONTH)) FROM t", @@ -87,6 +92,122 @@ class ScriptFunctionParenSpec extends AnyFlatSpec with Matchers { } } + "a scalar function wrapping a date-part extractor" should "parse (issue #220)" in { + // Assert the emitted script, never just `isRight`. The round trip is blind to this whole + // family: every rebalancing attempt that dropped `YEAR` from the function chain still rendered + // `.sql` with `YEAR(…)` in it, and the only visible symptom was the painless script losing + // `.get(ChronoField.YEAR)` — so a parse-succeeds assertion would have passed all three times. + Seq( + "ABS" -> "Math.abs", + "CEIL" -> "Math.ceil", + "FLOOR" -> "Math.floor", + "SQRT" -> "Math.sqrt", + "ROUND" -> "Math.round", + "SIGN" -> "arg0 > 0 ? 1", + "UPPER" -> "toUpperCase", + "LOWER" -> "toLowerCase" + ).foreach { case (fn, marker) => + val sql = s"SELECT $fn(YEAR(createdAt)) FROM t" + withClue(s"[$sql] ") { + identifierOf(sql).painless(None) should (include("ChronoField.YEAR") and include(marker)) + } + } + Seq( + "YEAR" -> "ChronoField.YEAR", + "MONTH" -> "ChronoField.MONTH_OF_YEAR", + "DAY" -> "ChronoField.DAY_OF_MONTH", + "WEEK" -> "IsoFields.WEEK_OF_WEEK_BASED_YEAR", + "QUARTER" -> "IsoFields.QUARTER_OF_YEAR", + "EPOCHDAY" -> "ChronoField.EPOCH_DAY", + "YEARDAY" -> "ChronoField.DAY_OF_YEAR", + "HOUR" -> "ChronoField.HOUR_OF_DAY", + "MINUTE" -> "ChronoField.MINUTE_OF_HOUR", + "SECOND" -> "ChronoField.SECOND_OF_MINUTE" + ).foreach { case (fn, marker) => + val sql = s"SELECT ABS($fn(createdAt)) FROM t" + withClue(s"[$sql] ") { + identifierOf(sql).painless(None) should (include(marker) and include("Math.abs")) + } + } + identifierOf("SELECT ABS(YEAR(DATE_TRUNC(createdAt, MONTH))) FROM t") + .painless(None) should (include("ChronoField.YEAR") and include("truncatedTo") and include( + "Math.abs" + )) + identifierOf("SELECT ROUND(ABS(YEAR(createdAt))) FROM t") + .painless(None) should (include("ChronoField.YEAR") and include("Math.abs") and include( + "Math.round" + )) + Parser("SELECT CAST(YEAR(createdAt) AS STRING) FROM t").isRight shouldBe true + } + + it should "leave a self-closing inner function alone" in { + // `opened == 0` — a single function that consumes its own parentheses is the whole match — is + // the one shape the new count hard-fails where `rep1(end)` used to succeed by stealing the + // enclosing `)`. Recovery depends on a sibling alternative existing, so pin it: these parsed + // before the fix and must still parse, with the inner transform intact. + identifierOf("SELECT ABS(DATE_TRUNC(createdAt, MONTH)) FROM t") + .painless(None) should (include("truncatedTo") and include("Math.abs")) + identifierOf("SELECT ABS(DATE_DIFF(a, b, DAY)) FROM t") + .painless(None) should (include("ChronoUnit.DAYS.between") and include("Math.abs")) + identifierOf("SELECT UPPER(DATE_TRUNC(createdAt, MONTH)) FROM t") + .painless(None) should (include("truncatedTo") and include("toUpperCase")) + } + + it should "leave a scalar wrapping an aggregate alone" in { + // `MAX` is a bare-name alternative too, so this shape sits next to the one being fixed. It + // never reaches the window branch of `Field.update` — the aggregate stays nested inside the + // math function rather than flattening into the chain — and it parsed identically before. + val id = identifierOf("SELECT ABS(MAX(salary)) AS m FROM t") + id.functions.map(_.getClass.getSimpleName) shouldBe List("MathematicalFunctionWithOp") + } + + "an aggregate wrapping an extractor" should "keep every transform in its function chain" in { + // Now that the parentheses balance, `MAX(YEAR(x))` reaches the same window-aggregate production + // as `MAX(x)` instead of the generic chain. `Field.update` then 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 — so `YEAR` vanished and the aggregation was scripted without it. + val id = identifierOf("SELECT MAX(YEAR(DATE_TRUNC(createdAt, MINUTE))) AS m FROM t GROUP BY id") + id.functions.map(_.getClass.getSimpleName) should contain allOf ("Year", "DateTrunc") + id.painless(None) should (include("ChronoField.YEAR") and include("truncatedTo")) + // And it is now the same aggregate the plain form produces — the two used to differ only + // because one argument happened to be parenthesis-balanced and the other was not. + id.functions.head.getClass shouldBe + identifierOf("SELECT MAX(salary) AS m FROM t GROUP BY id").functions.head.getClass + } + + "a postfix cast over an aggregate" should "stay a loud error, never a dropped cast" in { + // `MAX(YEAR(x))::STRING` was rejected before this change — the chain is + // `CastOperator :: MaxAgg :: Year` and the engine requires the aggregate to come first. Routing + // it through the window production must not turn that error into a silently un-cast column, so + // `Field.update` keeps whatever wraps the window instead of rebuilding from the window alone. + // The same error now covers the shapes that always took this path and silently swallowed the + // cast — `MAX(x)::T` returned a number where the user asked for a string. + Seq( + "SELECT MAX(YEAR(createdAt))::STRING AS m FROM t GROUP BY id", + "SELECT MAX(DATE_TRUNC(createdAt, MINUTE))::STRING AS m FROM t GROUP BY id", + "SELECT MAX(salary)::STRING AS m FROM t GROUP BY id", + "SELECT COUNT(id)::STRING AS c FROM t GROUP BY dept" + ).foreach { sql => + withClue(s"[$sql] ") { + Parser(sql).swap.toOption.map(_.msg) shouldBe Some( + "Aggregation function must be the first function in the chain" + ) + } + } + // The cast is still honoured wherever the aggregate is not in the way. + identifierOf("SELECT YEAR(createdAt)::STRING AS y FROM t").functions + .map(_.getClass.getSimpleName) shouldBe List("CastOperator", "Year") + } + + "a bare function name" should "not parse as a call with no arguments" in { + // Closing exactly what was opened makes zero parentheses an arithmetically valid count, so the + // production has to reject it explicitly — otherwise a naked keyword becomes a function applied + // to nothing, where the previous `rep1(end)` required a `)`. + Parser("SELECT MAX FROM t").isLeft shouldBe true + // `YEAR` on its own stays what it has always been: an ordinary column of that name. + identifierOf("SELECT YEAR FROM t").functions shouldBe empty + } + "a malformed script body" should "say so, instead of blaming a parenthesis elsewhere" in { // The scanner's own diagnostics only surface as `Error`: a `Failure` is discarded by both call // sites — `column`'s `script | optionalMultiFields` keeps the alternative's Success, and