ISSUE-2103 Handle trailing and trailing statement comments

cli/api/BUILD

@@ -58,10 +58,11 @@ node_modules(
 ts_test_suite(
     name = "tests",
     srcs = [
+        "tasks_test.ts",
+        "utils_test.ts",
         "commands/jit/rpc_test.ts",
         "dbadapters/bigquery_test.ts",
         "execution_sql_test.ts",
-        "utils_test.ts",
     ],
     data = [
         ":node_modules",

cli/api/dbadapters/tasks.ts

@@ -1,16 +1,53 @@
 import { dataform } from "df/protos/ts";
 
+function formatStatement(
+  statement: string,
+  isLast: boolean,
+  modifier?: (mod: string) => string
+) {
+  // Only treat a comment as trailing if it lives on the statement's last line.
+  // Searching from the last newline avoids misreading an interior comment (e.g.
+  // inside a multi-line procedure body) as a trailing comment.
+  const lastLineStart = statement.lastIndexOf("\n") + 1;
+  const commentIndex = statement.indexOf("--", lastLineStart);
+
+  let code: string;
+  let comment: string;
+
+  if (commentIndex === 0) {
+    code = "";
+    comment = statement;
+  } else if (commentIndex > 0) {
+    code = statement.substring(0, commentIndex).trimEnd();
+    comment = statement.substring(commentIndex);
+  } else {
+    code = statement;
+    comment = "";
+  }
+
+  if (modifier && code.length > 0) {
+    code = modifier(code);
+  }
+
+  if (!isLast && code.length > 0 && !code.endsWith(";")) {
+    code = code + ";";
+  }
+
+  if (code.length > 0 && comment.length > 0) {
+    return code + " " + comment;
+  }
+  return code || comment;
+}
+
 export function concatenateQueries(statements: string[], modifier?: (mod: string) => string) {
-  return statements
+  const processed = statements
     .filter(statement => !!statement)
     .map(statement => statement.trim())
-    .map(statement =>
-      statement.length > 0 && statement.charAt(statement.length - 1) === ";"
-        ? statement.substring(0, statement.length - 1)
-        : statement
-    )
-    .map(statement => (!!modifier ? modifier(statement) : statement))
-    .join("\n;\n");
+    .filter(statement => statement.length > 0);
+  const formattedStatements = processed.map((statement, index) =>
+    formatStatement(statement, index === processed.length - 1, modifier)
+  );
+  return formattedStatements.join("\n");
 }
 
 export class Tasks {

cli/api/goldens/on_schema_change_extend.sql

@@ -61,17 +61,15 @@ END IF;
 -- Cleanup temporary tables.
 DROP TABLE IF EXISTS `project-id.dataset-id.incremental_on_schema_change_df_temp_test_uuid_empty`;
 
-END
-;
+END;
 BEGIN
   CALL `project-id.dataset-id.df_osc_test_uuid`();
 EXCEPTION WHEN ERROR THEN
   DROP TABLE IF EXISTS `project-id.dataset-id.incremental_on_schema_change_df_temp_test_uuid_empty`;
   DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`;
   RAISE;
 END;
-DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`
-;
+DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`;
 insert into `project-id.dataset-id.incremental_on_schema_change`	
 (`id`,`field1`)	
 select `id`,`field1`	

cli/api/goldens/on_schema_change_fail.sql

@@ -52,17 +52,15 @@ END IF;
 -- Cleanup temporary tables.
 DROP TABLE IF EXISTS `project-id.dataset-id.incremental_on_schema_change_df_temp_test_uuid_empty`;
 
-END
-;
+END;
 BEGIN
   CALL `project-id.dataset-id.df_osc_test_uuid`();
 EXCEPTION WHEN ERROR THEN
   DROP TABLE IF EXISTS `project-id.dataset-id.incremental_on_schema_change_df_temp_test_uuid_empty`;
   DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`;
   RAISE;
 END;
-DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`
-;
+DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`;
 insert into `project-id.dataset-id.incremental_on_schema_change`	
 (`id`,`field1`)	
 select `id`,`field1`	

cli/api/goldens/on_schema_change_synchronize.sql

@@ -76,17 +76,15 @@ END IF;
 -- Cleanup temporary tables.
 DROP TABLE IF EXISTS `project-id.dataset-id.incremental_on_schema_change_df_temp_test_uuid_empty`;
 
-END
-;
+END;
 BEGIN
   CALL `project-id.dataset-id.df_osc_test_uuid`();
 EXCEPTION WHEN ERROR THEN
   DROP TABLE IF EXISTS `project-id.dataset-id.incremental_on_schema_change_df_temp_test_uuid_empty`;
   DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`;
   RAISE;
 END;
-DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`
-;
+DROP PROCEDURE IF EXISTS `project-id.dataset-id.df_osc_test_uuid`;
 merge `project-id.dataset-id.incremental_on_schema_change` T
 using (select 1 as id, 'a' as field1, 'new' as field2
 ) S

cli/api/tasks_test.ts

@@ -0,0 +1,89 @@
+import { expect } from "chai";
+
+import { concatenateQueries } from "df/cli/api/dbadapters/tasks";
+import { suite, test } from "df/testing";
+
+suite("concatenateQueries", () => {
+  test("adds semicolons to non-last statements and joins with newlines", () => {
+    expect(concatenateQueries(["SELECT 1", "SELECT 2"])).deep.equals(
+      "SELECT 1;\nSELECT 2"
+    );
+  });
+
+  test("does not add semicolon to the last statement", () => {
+    expect(concatenateQueries(["SELECT 1", "SELECT 2", "SELECT 3"])).deep.equals(
+      "SELECT 1;\nSELECT 2;\nSELECT 3"
+    );
+  });
+
+  test("preserves existing semicolons on non-last statements", () => {
+    expect(concatenateQueries(["SELECT 1;", "SELECT 2"])).deep.equals(
+      "SELECT 1;\nSELECT 2"
+    );
+  });
+
+  test("preserves existing semicolons on last statement", () => {
+    expect(concatenateQueries(["SELECT 1;", "SELECT 2;"])).deep.equals(
+      "SELECT 1;\nSELECT 2;"
+    );
+  });
+
+  test("adds semicolon before trailing comment if necessary, verifies fix for https://github.com/dataform-co/dataform/issues/1231", () => {
+    expect(concatenateQueries(["SELECT 1 -- blah", "drop table foo; -- baz", "SELECT 2"])).deep.equals(
+      "SELECT 1; -- blah\ndrop table foo; -- baz\nSELECT 2"
+    );
+  });
+
+  test("preserves comment-only lines, verifies fix for https://github.com/dataform-co/dataform/issues/2103", () => {
+    expect(concatenateQueries(["SELECT 1;", "--drop table foo", "SELECT 2", "--baz"])).deep.equals(
+      "SELECT 1;\n--drop table foo\nSELECT 2;\n--baz"
+    );
+  });
+
+  test("preserves single line comments within a query", () => {
+    expect(concatenateQueries(["SELECT 1\n--test\nFROM UNNEST([]);"])).deep.equals(
+      "SELECT 1\n--test\nFROM UNNEST([]);"
+    );
+  });
+
+  test("preserves multi line comments within a query", () => {
+    expect(concatenateQueries(["SELECT 1\n\*multi\nline\ncomment\n*/\nFROM UNNEST([]);"])).deep.equals(
+      "SELECT 1\n\*multi\nline\ncomment\n*/\nFROM UNNEST([]);"
+    );
+  });
+
+  test("trims whitespace from statements", () => {
+    expect(concatenateQueries(["  SELECT 1  ", "  SELECT 2  ", "  SELECT 3 -- blah"])).deep.equals(
+      "SELECT 1;\nSELECT 2;\nSELECT 3 -- blah"
+    );
+  });
+
+  test("filters out empty and falsy statements", () => {
+    expect(concatenateQueries(["SELECT 1", "", "SELECT 2"])).deep.equals(
+      "SELECT 1;\nSELECT 2"
+    );
+    expect(concatenateQueries(["SELECT 1", undefined, "SELECT 2"])).deep.equals(
+      "SELECT 1;\nSELECT 2"
+    );
+  });
+
+  test("returns empty string for empty input", () => {
+    expect(concatenateQueries([])).deep.equals("");
+  });
+
+  test("returns single statement without semicolon", () => {
+    expect(concatenateQueries(["SELECT 1"])).deep.equals("SELECT 1");
+  });
+
+  test("applies modifier to code portion of each statement", () => {
+    expect(
+      concatenateQueries(["SELECT 1", "SELECT 2"], stmt => `(${stmt})`)
+    ).deep.equals("(SELECT 1);\n(SELECT 2)");
+  });
+
+  test("applies modifier only to code portion, not comment", () => {
+    expect(
+      concatenateQueries(["SELECT 1 --comment", "SELECT 2"], stmt => `(${stmt})`)
+    ).deep.equals("(SELECT 1); --comment\n(SELECT 2)");
+  });
+});

tests/api/api.spec.ts

@@ -225,7 +225,7 @@ suite("@dataform/api", () => {
           expect(action.tasks).eql([
             dataform.ExecutionTask.create({
               type: "statement",
-              statement: "preOp\n;\ncreate or replace table `schema.a` as foo\n;\npostOp"
+              statement: "preOp;\ncreate or replace table `schema.a` as foo;\npostOp"
             })
           ]);
         });
@@ -242,7 +242,7 @@ suite("@dataform/api", () => {
             dataform.ExecutionTask.create({
               type: "statement",
               statement:
-                "incremental preOp\n;\ndrop view if exists `schema.a`\n;\ninsert into `schema.a`\t\n()\t\nselect \t\nfrom (incremental foo) as insertions\n;\nincremental postOp"
+                "incremental preOp;\ndrop view if exists `schema.a`;\ninsert into `schema.a`\t\n()\t\nselect \t\nfrom (incremental foo) as insertions;\nincremental postOp"
             })
           ]);
         });

tests/integration/bigquery.spec.ts

@@ -35,7 +35,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after })
       const executedGraph = await dfapi.run(dbadapter, executionGraph).result();
 
       const actionMap = keyBy(executedGraph.actions, v => targetAsReadableString(v.target));
-      expect(Object.keys(actionMap).length).eql(17);
+      expect(Object.keys(actionMap).length).eql(20);
 
       // Check the status of action execution.
       const expectedFailedActions = [
@@ -384,7 +384,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after })
       const bqadapter = new ExecutionSql({ warehouse: "bigquery" }, "1.4.8");
 
       const refresh = bqadapter.publishTasks(table, { fullRefresh: true }, { fields: [] }).build();
-      const splitRefresh = refresh[0].statement.split("\n;\n");
+      const splitRefresh = refresh[0].statement.split("\n");
       expect([...splitRefresh.slice(0, 2), ...splitRefresh.slice(-2)]).to.eql([
         ...table.preOps,
         ...table.postOps
@@ -394,7 +394,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after })
         .publishTasks(table, { fullRefresh: false }, { fields: [] })
         .build();
 
-      const splitIncrement = increment[0].statement.split("\n;\n");
+      const splitIncrement = increment[0].statement.split("\n");
       expect([...splitIncrement.slice(0, 2), ...splitIncrement.slice(-2)]).to.eql([
         ...table.preOps,
         ...table.postOps

tests/integration/bigquery_project/definitions/example_internal_comment_in_query.sqlx

@@ -0,0 +1,24 @@
+config { 
+    type: "table", 
+    description: "Verifies fix for https://github.com/dataform-co/dataform/issues/1231" 
+} 
+
+post_operations { 
+    select *
+    -- single line comment
+    /*
+    multi
+    line
+    comment
+    */ 
+    from ${self()} 
+}
+
+select 1 as col1 
+-- single line comment
+/*
+multi
+line
+comment
+*/
+FROM UNNEST([])

tests/integration/bigquery_project/definitions/example_trailing_comment_in_operations.sqlx

@@ -0,0 +1,13 @@
+config {
+  "type": "table",
+  "description": "Verifies fix for https://github.com/dataform-co/dataform/issues/2103"
+}
+
+SELECT 1 AS foo
+
+pre_operations {
+
+  DECLARE bar DATE DEFAULT DATE_TRUNC(CURRENT_DATE(), WEEK(MONDAY));
+
+  -- DECLARE bar DATE DEFAULT DATE_TRUNC(DATE('2026-01-01'), WEEK(MONDAY))
+}

tests/integration/bigquery_project/definitions/example_trailing_statement_comment_with_operations.sqlx

@@ -0,0 +1,12 @@
+config { 
+    type: "table", 
+    schema: "test", 
+    name: "testtab",
+    description: "Verifies fix for https://github.com/dataform-co/dataform/issues/1231" 
+} 
+
+post_operations { 
+    select * from ${self()} 
+}
+
+select 1 as col1 -- test comment

tools/ts_proto_library.bzl

@@ -31,7 +31,7 @@ def _run_pbts(actions, executable, js_file):
     #
     # Run pbts and then patch 'import Long = require("long");' to 'import Long from "long";'
     # to avoid syntax errors in older rollup-plugin-dts.
-    command = "{pbts} --out {out} {input} && sed -i 's/import Long = require(\"long\");/import Long from \"long\";/g' {out}".format(
+    command = "{pbts} --out {out} {input} && sed 's/import Long = require(\"long\");/import Long from \"long\";/g' {out} > {out}.tmp && mv {out}.tmp {out}".format(
         pbts = executable._pbts.path,
         out = ts_file.path,
         input = js_file.path