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

[fupelaqu]

SELECT ABS(YEAR(createdAt)) FROM t did not parse, and the error named a parenthesis at the FROM: ')' expected but 'F' found.

What was wrong

identifierWithFunction parses a nested call as a chain of bare function names separated by ( — rep1sep(sql_function, start) — then the innermost identifier, then the closing parens. It closed with rep1(end): one or MORE. Nothing tied the number of ) consumed to the number of ( opened, so a call to a name-only extractor ate the parenthesis belonging to whatever enclosed it.

It now closes exactly what it opened — one ) per rep1sep separator, plus the optional one introducing the innermost identifier:

val opened = f.size - 1 + (if (s.isDefined) 1 else 0)
if (opened < 1) failure("function call expected") else repN(opened, end) ^^ { … }

Zero is now an arithmetically valid count, so it has to be rejected explicitly — the old rep1 refused it as a side effect of demanding a ), and without the guard SELECT MAX FROM t would parse as a function applied to nothing. failure rather than err, so an alternative stays free to read the token as an ordinary column: SELECT YEAR FROM t is a column named year.

Only the name-only extractors were affected (YEAR, MONTH, DAY, WEEK, QUARTER, EPOCHDAY, YEARDAY, HOUR, MINUTE, SECOND), because they are declared as the bare name and so reach the generic path. DATE_TRUNC, DATE_DIFF, WEEKDAY, ABS, UPPER consume their own parentheses and never were. That is why the failure looked arbitrary — and why SELECT YEAR(x) always worked: nothing encloses it there.

This is the general case of the boundary #219 fixed locally for SCRIPT AS.

The shape change, and the two latent bugs it exposed

With the parentheses balanced, MAX(YEAR(x)) reaches the same window-aggregate production as MAX(x) and yields MaxAgg, where it used to fall through to the generic chain and yield the plain Max object. The two only ever differed because one argument happened to be parenthesis-balanced and the other was not.

That newly-taken path was carrying two silent bugs, both in Field.update, which rebuilt the chain from the window's identifier alone. A field's chain is <applied after the window> :: window :: <applied before it> and both ends were lost:

• The trailing end. windowFunction +: windowFunction.identifier.functions.tail drops a head that is only the window function when the list is the field's own. Since 747553d0 the list has been the window's identifier's, whose head is the innermost transform. MAX(YEAR(DATE_TRUNC(x, MINUTE))) lost YEAR and was scripted without .get(ChronoField.YEAR).
• The leading end. Rebuilding from the window's identifier cannot express anything wrapping the window. A postfix cast parses to CastOperator :: MaxAgg :: …, so MAX(salary)::STRING came back un-cast, the round trip quietly shorter than the statement.

⚠️ Behaviour changes to note at release

AGG(…)::TYPE is now rejected instead of silently ignoring the cast. The engine requires the aggregate to be first in the chain, and a postfix cast puts something ahead of it — MAX(YEAR(x))::STRING was already rejected on main for exactly that reason. Preserving the leading end is what keeps it rejected; without it this PR would have downgraded that loud error into a silently un-cast column. The same error now also covers MAX(x)::T and MAX(DATE_TRUNC(x))::T, which previously parsed and dropped the cast — returning a number where the user asked for a string. Nothing in the test corpus used the shape.

Aggregations now come out in SELECT order. bridge's datetime_parse expectation moved from {lastSeen, ct} to {ct, lastSeen}; its date_parse sibling — the same query with a balanced inner function — already expected SELECT order, so this removes an inconsistency rather than creating one. Measured in both directions and for both balanced and unbalanced inner functions. Elasticsearch does not care about aggs key order, but any downstream consumer pinning generated JSON for an aggregate over a bare-name extractor needs the two keys swapped. The three sibling repos were checked: none pins aggregation JSON, and the two places touching this AST (RequiredField.aggregation: Option[AggregateFunction], JoinPlanner's empty-chain SELECT * test) are unaffected.

Not fixed here

• SELECT ABS(YEAR(x) + 1) still fails — but so does SELECT ABS(salary + 1). A scalar function's argument is valueExpr, which has no arithmetic alternative; separate, pre-existing, unrelated to this boundary.
• MAX() still parses as a call with no argument. The new guard only rejects a bare function name; an empty argument list is a different, pre-existing acceptance.
• Two pre-existing silent-wrong-answer defects the review surfaced in shapes this fix newly routes through the window path, now filed as STDDEV / VARIANCE over a transform silently compute the statistic on the raw field (or emit an empty extended_stats) #222 (STDDEV(YEAR(x)) emits extended_stats over the raw date with no script) and HAVING / ORDER BY on an aggregate over a transform emit a malformed aggregation (double transform + doc[] in bucket_selector; empty agg name) #223 (HAVING MAX(YEAR(x)) > … emits a bucket_selector with the transform applied twice and an illegal doc[…] reference; ORDER BY MAX(ABS(x)) names the aggregation ""). Both were verified byte-identical against the pre-fix sources, so neither is caused by this change.

Documentation

The last commit documents the cast restriction in documentation/sql/type_conversion.md and functions_type_conversion.md. It also corrects the cast target list, which was measured against the parser rather than trusted: DECIMAL(p,s) / NUMERIC(p,s), TEXT and BOOL were all advertised as cast targets and none of them parse, while CHAR works and was missing. Three examples that omitted FROM were fixed too — every statement requires one, so they could not have run as written.

Companion public page: softclient4es-web#38.

Verification

sql 500 on Scala 2.12 and 2.13 · core 737 · macrosTests 18 · bridge template and all four es{6,7,8,9} bridges 120 each (es9 under sbt17) · Docker integration on real ES6/7/8/9: ES8 284 passed / 3 canceled across 19 suites, ES6 55/1, ES7 56, ES9 56 · scalafmtCheckAll + headerCheck clean.

The new tests assert the emitted painless script and the function chain, not the .sql round trip — the round trip is blind to this entire family, which is how three earlier rebalancing attempts passed sql/test at 487 while dropping YEAR.

Closes #220

🤖 Generated with Claude Code