fix(sql): make ALTER COLUMN ... SET|ADD FIELD parse and actually apply

[fupelaqu]

ALTER COLUMN c SET FIELD raw KEYWORD is documented, but only ADD ever parsed — and even ADD FIELD changed nothing. Two independent bugs stacked on the same statement, both silent.

The parse

multiFields carried its own | success(Nil) fallback, so every consumer became optional. alterColumnFields is literally ALTER COLUMN c SET <multiFields>, so it matched a bare SET with an empty field list and left FIELD raw KEYWORD unconsumed:

SQL as written	Before	After
ALTER COLUMN c SET FIELD raw KEYWORD	matched SET + nothing; the rest silently discarded pre-#213, parse error after	parses
ALTER COLUMN c ADD FIELD raw KEYWORD	parses	parses (same AST — SET/ADD are synonyms)
ALTER COLUMN c SET	parsed as AlterColumnFields(c, Nil) — a statement that did nothing	rejected
ALTER COLUMN c SET FIELDS (…)	parses	parses (unchanged)

The fallback moved to optionalMultiFields, whose only caller is the column definition that legitimately allows a column without sub-fields. (?i)FIELDS\b cannot match FIELD raw and (?i)FIELD\b cannot match FIELDS (, so both statements stay reachable regardless of alternation order.

The apply — found by the adversarial review, and the reason this is not a one-line PR

Making the statement parse would have been worse than leaving it broken: Table.merge's single-field branches computed their copy and threw it away, returning the unchanged table.

val updatedFields = c.multiFields.filterNot(_.name == field.name) :+ field
c.copy(multiFields = updatedFields)   // result discarded
table                                  // unchanged table returned

Value discarding, the same family as the #205 bridge chain. GatewayApi.run does merge → diff → push, so an unchanged merge means an empty diff: nothing reached Elasticsearch and the DDL still reported success. DROP FIELD had the identical bug. Both now write the column back, mirroring the AlterColumnFields branch they were modelled on (field.update(Some(c)) + updateStruct()).

Round-trip

AlterColumnField.sql renders SET FIELD, so while SET FIELD was unparseable the AST could not re-parse its own rendering — the path TableDiff uses for FieldAdded/FieldAltered. Fixed by the parse change; the renderer now interpolates ${field.sql} instead of relying on Token.toString.

Docs

documentation/sql/ddl_statements.md, every example parse-probed through the real parser:

• table-level MAPPING/SETTING take a bare key = value — the parentheses in the old text belong to an object value (SET MAPPING _meta = (owner = 'analytics')), and the documented form never parsed
• SET|ADD ALIAS / DROP ALIAS exist in the grammar and were undocumented
• CREATE TABLE … OPTIONS (…) section replaces a WITH (…) clause the grammar never had — the enrich walkthrough's default_pipeline was silently dropped, so following that guide produced a table whose inserts never went through the pipeline the walkthrough exists to demonstrate

softclient4es-web carries the matching SET MAPPING/SET SETTING correction on the existing sync branch (PR #37).

Verification

sql 464 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) · scalafmtCheckAll + headerCheck clean.

Verified to fail without their fix: 3 of the 5 new ParserSpec tests, and 5 of the 6 new AlterTableMergeSpec tests — the plural SET FIELDS control stays green, as it must.

⚠️ Release-note item

ALTER COLUMN … SET|ADD FIELD and DROP FIELD now change the index. They previously parsed (or not) and did nothing while reporting success, so a schema that "already had" those statements applied will now actually receive them.

Second pass — the rest of the family

The remaining render/parse asymmetries, plus two defects the review of this PR turned up. AlterColumnType (SET TYPE vs SET DATA TYPE) and the AlterTable/AlterPipeline IF EXISTS position were fixed by @smanciot; the round-trip spec below covers them.

Defect	Effect
SET DEFAULT _ingest.timestamp renderable but not parseable	the MV _last_updated column breaks when it already exists; option had the same gap, which is how _meta.columns.<c>.default_value is written
string literals rendered unescaped	SET COMMENT 'it's here' cannot be parsed back
nine throw inside combinator actions	escaped Parser.apply's Either[ParserError, Statement] signature
Table.merge resolved against this, not the fold accumulator	ALTER TABLE users (ADD COLUMN profile VARCHAR, ALTER COLUMN profile SET FIELDS (…)) threw ColumnNotFound
ObjectValue.set/remove collapsed paths of depth ≥ 3	setting _meta.columns.<c>.default_value replaced the whole of _meta with its innermost object
terms include/exclude derived from v.sql	quote delimiters embedded in LIKE/RLIKE regexes; escaped literals would have sent a\\b to ES for a one-backslash value
help JSON SET TYPE	shipped syntax the grammar rejects

The last two are why this pass matters beyond tidiness. The ObjectValue bug is pre-existing, but the ingest-default fix above would have turned it from a loud failure into a silent corruption — the statement now parses, so it would have run and quietly emptied _meta. Measured before the fix: applying the ALTER that TableDiff itself produced left a non-empty residual diff (MappingRemoved(_meta.not_null), _meta.data_type, _meta.default_value), i.e. an MV reconcile loop that never converges. The include/exclude one is a regression this PR's own escaping change introduced into a live ES query path, caught by the review.

Guards

This family has been found one member at a time, so the tests enumerate rather than sample:

• AlterTableRoundTripSpec — 33 ALTER TABLE statements + ALTER PIPELINE + quote/backslash cases + the guards that must return Left rather than throw
• ObjectValuePathSpec — depth-3 set/remove, and that removing an absent path is a no-op
• BucketIncludesSpec — the ES include/exclude derivation
• AlterTableMergeSpec — the accumulator cases and a convergence test: applying the ALTER a diff produced must leave no residual diff

All verified to fail without their fix.

Verification

sql 482 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) · scalafmtCheckAll + headerCheck clean.

⚠️ Release-note items

• ALTER COLUMN … SET|ADD FIELD / DROP FIELD now change the index; they previously reported success and did nothing.
• HAVING <grouped field> LIKE|RLIKE '…' no longer embeds the quote characters in the generated terms regex, so an include that never matched now matches.

Still open, deliberately not fixed here

• YEAR(…) inside SCRIPT AS does not parse at all — ALTER COLUMN age SET SCRIPT AS (YEAR(CURRENT_DATE) - YEAR(birthdate)) and the same expression in a CREATE TABLE column both fail, while DATE_DIFF(birthdate, CURRENT_DATE, YEAR) works. That exact expression is a published example in the web docs.
• Nested ingest placeholders coerce to strings on a round trip ((routing = _id) renders (routing = "_id")). Left as is: the _meta JSON read-back already reconstructs a StringValue, so quoting matches how Elasticsearch stores it.
• CharValue.sql is still unescaped; unreachable from the parser.

🤖 Generated with Claude Code