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 f5e91847..267a64f8 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 @@ -247,8 +247,72 @@ object Parser identifierWithIntervalFunction | identifierWithFunction + /** 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. + * + * 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. + */ + private def scriptBody: Parser[String] = new Parser[String] { + def apply(in: Input): ParseResult[String] = { + val source = in.source + var i = in.offset + while (i < source.length && source.charAt(i).isWhitespace) i += 1 + // `Error`, not `Failure`: `SCRIPT AS` has already been consumed, so no alternative is + // legitimate from here. A `Failure` is discarded by both call sites for structural reasons — + // `column`'s `script | optionalMultiFields` keeps the alternative's Success and drops the + // deeper failure, and `alterTable`'s `repsep` accepts zero statements so `phrase` overwrites + // the message — which left the diagnostic below unreachable and reported the very message + // this production exists to eliminate. + if (i >= source.length || source.charAt(i) != '(') + Error("'(' expected after SCRIPT AS", in) + else { + val bodyStart = i + 1 + var depth = 0 + var quote: Char = 0 + var closing = -1 + while (i < source.length && closing < 0) { + val c = source.charAt(i) + if (quote != 0) { + if (c == '\\') i += 1 + else if (c == quote) quote = 0 + } else + c match { + case '\'' | '"' => quote = c + case '(' => depth += 1 + case ')' => depth -= 1; if (depth == 0) closing = i + case _ => () + } + i += 1 + } + if (closing < 0) Error("unbalanced parentheses in SCRIPT AS", in) + else + Success( + source.subSequence(bodyStart, closing).toString, + in.drop(closing + 1 - in.offset) + ) + } + } + } + def script: PackratParser[PainlessScript] = - (keyword("SCRIPT") ~ keyword("AS")) ~ start ~ scriptValue ~ end ^^ { case _ ~ _ ~ s ~ _ => s } + (keyword("SCRIPT") ~ keyword("AS")) ~> scriptBody >> { body => + parseAll(scriptValue, body) match { + case Success(s, _) => success(s) + case NoSuccess(msg, _) => err(s"Invalid SCRIPT AS expression ($body): $msg") + } + } def column: PackratParser[Column] = ident ~ extension_type ~ (script | optionalMultiFields) ~ defaultVal ~ notNull ~ comment ~ (options | success( diff --git a/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala b/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala index 00c07e1c..bf2a258e 100644 --- a/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala +++ b/sql/src/main/scala/app/softnetwork/elastic/sql/schema/package.scala @@ -1602,7 +1602,13 @@ package object schema { table.copy( columns = table.columns.map { col => if (col.name == columnName) - col.copy(script = Some(newScript.copy(dataType = col.dataType))) + // A column is either multi-field or script-defined, never both — the grammar + // says so (`ident ~ extension_type ~ (script | optionalMultiFields)`), so a + // column carrying both renders DDL that cannot be parsed back. + col.copy( + script = Some(newScript.copy(dataType = col.dataType)), + multiFields = Nil + ) else col } ) @@ -1737,9 +1743,11 @@ package object schema { else { col match { case Some(c) => + // …and symmetrically: declaring sub-fields drops a script the column had. val updated = c .copy( - multiFields = newFields.toList.map(_.update(Some(c))) + multiFields = newFields.toList.map(_.update(Some(c))), + script = None ) .updateStruct() table.copy( @@ -1766,7 +1774,8 @@ package object schema { val updated = c .copy( multiFields = - c.multiFields.filterNot(_.name == field.name) :+ field.update(Some(c)) + c.multiFields.filterNot(_.name == field.name) :+ field.update(Some(c)), + script = None ) .updateStruct() table.copy( @@ -1876,6 +1885,14 @@ package object schema { errors = errors :+ s"Partition column ${partition.column} does not exist in table $name" } } + // A column is either multi-field or script-defined, never both: the grammar offers one or the + // other (`ident ~ extension_type ~ (script | optionalMultiFields)`), so such a column renders + // `name TEXT FIELDS (…) SCRIPT AS (…)`, which cannot be parsed back — and that rendering is + // what SHOW CREATE TABLE and the diff statements emit. Loud here beats invalid DDL there. + columns.filter(c => c.script.isDefined && c.multiFields.nonEmpty).foreach { c => + errors = errors :+ + s"Column ${c.name} of table $name cannot declare both FIELDS and SCRIPT AS" + } if (errors.isEmpty) Right(()) else Left(errors.mkString("\n")) } 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 new file mode 100644 index 00000000..d905a316 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/parser/ScriptFunctionParenSpec.scala @@ -0,0 +1,114 @@ +package app.softnetwork.elastic.sql.parser + +import app.softnetwork.elastic.sql.query.{AlterTable, CreateTable} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** `identifierWithFunction` closed with `rep1(end)` — one or more parentheses — so it consumed a + * `)` belonging to whatever production enclosed it. Inside `SCRIPT AS ( … )` that is the wrapper's + * own closing paren, which made every name-only extractor function unusable in a generated column: + * `age INT SCRIPT AS (YEAR(CURRENT_DATE) - YEAR(birthdate))` — the published example — could not + * be parsed, while `SELECT YEAR(x)` always worked because nothing enclosed it. + */ +class ScriptFunctionParenSpec extends AnyFlatSpec with Matchers { + + private def scriptOf(sql: String): String = + Parser(sql) match { + case Right(CreateTable(_, Right(columns), _, _, _, _, _)) => + columns + .find(_.script.isDefined) + .flatMap(_.script.map(_.sql)) + .getOrElse(fail(s"no scripted column in [$sql]")) + case other => fail(s"Expected a CreateTable, got $other") + } + + "the documented CREATE TABLE example" should "parse" in { + val sql = + """CREATE TABLE users ( + | id INT, + | birthdate DATE, + | age INT SCRIPT AS (YEAR(CURRENT_DATE) - YEAR(birthdate)), + | PRIMARY KEY (id) + |)""".stripMargin + Parser(sql).isRight shouldBe true + scriptOf(sql) should include("YEAR") + } + + "every date-part extractor" should "be usable inside SCRIPT AS" in { + // Name-only extractors — the family that reaches the generic function path. + Seq( + "YEAR", + "MONTH", + "DAY", + "WEEK", + "QUARTER", + "EPOCHDAY", + "YEARDAY", + "HOUR", + "MINUTE", + "SECOND" + ) + .foreach { fn => + val sql = s"ALTER TABLE users ALTER COLUMN part SET SCRIPT AS ($fn(birthdate))" + withClue(s"[$sql] ") { Parser(sql).isRight shouldBe true } + } + } + + it should "still work for the functions that were never affected" in { + Seq( + "ALTER TABLE users ALTER COLUMN d SET SCRIPT AS (WEEKDAY(birthdate))", + "ALTER TABLE users ALTER COLUMN a SET SCRIPT AS (ABS(salary))", + "ALTER TABLE users ALTER COLUMN n SET SCRIPT AS (UPPER(name))", + "ALTER TABLE users ALTER COLUMN g SET SCRIPT AS (DATE_DIFF(birthdate, CURRENT_DATE, YEAR))", + "ALTER TABLE users ALTER COLUMN p SET SCRIPT AS (birthdate + 1)" + ).foreach { sql => + withClue(s"[$sql] ") { Parser(sql).isRight shouldBe true } + } + } + + "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", + "SELECT MAX(YEAR(DATE_TRUNC(createdAt, MINUTE))) AS m FROM t GROUP BY id", + "SELECT YEAR(CURRENT_DATE) - YEAR(birthdate) FROM t" + ).foreach { sql => + withClue(s"[$sql] ") { Parser(sql).isRight shouldBe true } + } + // … and where one does. + Seq( + "ALTER TABLE t ALTER COLUMN c SET SCRIPT AS (YEAR(DATE_TRUNC(createdAt, MONTH)))", + "ALTER TABLE t ALTER COLUMN c SET SCRIPT AS (DATE_TRUNC(createdAt, MONTH))" + ).foreach { sql => + withClue(s"[$sql] ") { Parser(sql).isRight shouldBe true } + } + } + + "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 + // `alterTable`'s `repsep` accepts zero statements — which reported `')' expected but 'S' + // found`, the exact symptom this production exists to eliminate. + val unbalanced = Parser("CREATE TABLE t (a INT SCRIPT AS (b + 1") + unbalanced.isLeft shouldBe true + unbalanced.swap.toOption.get.msg should include("unbalanced parentheses in SCRIPT AS") + + val noParen = Parser("CREATE TABLE t (a INT SCRIPT AS b)") + noParen.isLeft shouldBe true + noParen.swap.toOption.get.msg should include("'(' expected after SCRIPT AS") + + val badBody = Parser("ALTER TABLE t ALTER COLUMN a SET SCRIPT AS (@@@)") + badBody.isLeft shouldBe true + badBody.swap.toOption.get.msg should include("Invalid SCRIPT AS expression") + } + + "a scripted column" should "re-parse the SQL it renders" in { + val sql = "ALTER TABLE users ALTER COLUMN age SET SCRIPT AS (YEAR(birthdate))" + val stmt = Parser(sql).toOption.getOrElse(fail(s"did not parse: $sql")) + stmt shouldBe a[AlterTable] + Parser(stmt.sql) shouldBe Right(stmt) + } +} diff --git a/sql/src/test/scala/app/softnetwork/elastic/sql/schema/ColumnScriptFieldsExclusionSpec.scala b/sql/src/test/scala/app/softnetwork/elastic/sql/schema/ColumnScriptFieldsExclusionSpec.scala new file mode 100644 index 00000000..60067250 --- /dev/null +++ b/sql/src/test/scala/app/softnetwork/elastic/sql/schema/ColumnScriptFieldsExclusionSpec.scala @@ -0,0 +1,99 @@ +package app.softnetwork.elastic.sql.schema + +import app.softnetwork.elastic.sql.`type`.SQLTypes +import app.softnetwork.elastic.sql.parser.Parser +import app.softnetwork.elastic.sql.query.{AlterTable, AlterTableStatement} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +/** A column is either multi-field or script-defined, never both — the grammar offers one or the + * other (`ident ~ extension_type ~ (script | optionalMultiFields)`). `Table.merge` did not honour + * that: `ALTER COLUMN … SET SCRIPT AS` kept the existing sub-fields, and the resulting `Table.sql` + * — what SHOW CREATE TABLE and the diff statements emit — rendered `name TEXT FIELDS (…) SCRIPT AS + * (…)`, which cannot be parsed back. + */ +class ColumnScriptFieldsExclusionSpec extends AnyFlatSpec with Matchers { + + private def statementsOf(sql: String): Seq[AlterTableStatement] = + Parser(sql) match { + case Right(AlterTable(_, _, statements)) => statements + case other => fail(s"Expected an AlterTable, got $other") + } + + private val withFields: Table = + Parser("CREATE TABLE t (id INT, name TEXT FIELDS (raw KEYWORD), PRIMARY KEY (id))") match { + case Right(ct: app.softnetwork.elastic.sql.query.CreateTable) => + Table( + name = ct.table, + columns = ct.ddl.getOrElse(fail("expected columns")), + primaryKey = ct.primaryKey + ).update() + case other => fail(s"Expected a CreateTable, got $other") + } + + private def columnOf(table: Table, name: String): Column = + table.columns.find(_.name == name).getOrElse(fail(s"no column $name")) + + "setting a script" should "drop the sub-fields the column had" in { + val merged = + withFields.merge(statementsOf("ALTER TABLE t ALTER COLUMN name SET SCRIPT AS (UPPER(name))")) + val c = columnOf(merged, "name") + c.script shouldBe defined + c.multiFields shouldBe empty + } + + "declaring sub-fields" should "drop a script the column had" in { + val scripted = + withFields.merge(statementsOf("ALTER TABLE t ALTER COLUMN name SET SCRIPT AS (UPPER(name))")) + val refielded = + scripted.merge(statementsOf("ALTER TABLE t ALTER COLUMN name SET FIELDS (raw KEYWORD)")) + val c = columnOf(refielded, "name") + c.multiFields.map(_.name) should contain("raw") + c.script shouldBe empty + } + + it should "drop a script when a single field is added" in { + val scripted = + withFields.merge(statementsOf("ALTER TABLE t ALTER COLUMN name SET SCRIPT AS (UPPER(name))")) + val refielded = + scripted.merge(statementsOf("ALTER TABLE t ALTER COLUMN name ADD FIELD raw KEYWORD")) + columnOf(refielded, "name").script shouldBe empty + } + + "the merged table" should "still render DDL that parses" in { + val merged = + withFields.merge(statementsOf("ALTER TABLE t ALTER COLUMN name SET SCRIPT AS (UPPER(name))")) + withClue(s"rendered [${merged.sql}] ") { Parser(merged.sql).isRight shouldBe true } + } + + "a table holding an impossible column" should "fail validation rather than render invalid DDL" in { + // Reachable outside SQL — `IndexField.ddlColumn` fills `script` and `multiFields` from a live + // mapping independently. + val impossible = withFields.copy(columns = withFields.columns.map { + case c if c.name == "name" => + c.copy(script = + columnOf( + withFields.merge( + statementsOf("ALTER TABLE t ALTER COLUMN name SET SCRIPT AS (UPPER(name))") + ), + "name" + ).script + ) + case c => c + }) + impossible.columns.find(_.name == "name").exists(_.multiFields.nonEmpty) shouldBe true + impossible.validate().isLeft shouldBe true + impossible.validate().swap.toOption.get should include( + "cannot declare both FIELDS and SCRIPT AS" + ) + } + + "a plain column" should "still validate" in { + withFields.validate() shouldBe Right(()) + Table(name = "u", columns = List(Column("id", SQLTypes.Int))) + .update() + .validate() shouldBe Right( + () + ) + } +}