diff --git a/cli/api/BUILD b/cli/api/BUILD index 111ff2a89..d989cfe2c 100644 --- a/cli/api/BUILD +++ b/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", diff --git a/cli/api/dbadapters/tasks.ts b/cli/api/dbadapters/tasks.ts index 8991e5bed..353fcaab4 100644 --- a/cli/api/dbadapters/tasks.ts +++ b/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 { diff --git a/cli/api/goldens/on_schema_change_extend.sql b/cli/api/goldens/on_schema_change_extend.sql index 21f9558a8..de070956e 100644 --- a/cli/api/goldens/on_schema_change_extend.sql +++ b/cli/api/goldens/on_schema_change_extend.sql @@ -61,8 +61,7 @@ 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 @@ -70,8 +69,7 @@ EXCEPTION WHEN ERROR THEN 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` diff --git a/cli/api/goldens/on_schema_change_fail.sql b/cli/api/goldens/on_schema_change_fail.sql index ea8d90542..85ad5ed22 100644 --- a/cli/api/goldens/on_schema_change_fail.sql +++ b/cli/api/goldens/on_schema_change_fail.sql @@ -52,8 +52,7 @@ 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 @@ -61,8 +60,7 @@ EXCEPTION WHEN ERROR THEN 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` diff --git a/cli/api/goldens/on_schema_change_synchronize.sql b/cli/api/goldens/on_schema_change_synchronize.sql index bcfc63eac..ea6bf461b 100644 --- a/cli/api/goldens/on_schema_change_synchronize.sql +++ b/cli/api/goldens/on_schema_change_synchronize.sql @@ -76,8 +76,7 @@ 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 @@ -85,8 +84,7 @@ EXCEPTION WHEN ERROR THEN 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 diff --git a/cli/api/tasks_test.ts b/cli/api/tasks_test.ts new file mode 100644 index 000000000..2510e6d6a --- /dev/null +++ b/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)"); + }); +}); diff --git a/tests/api/api.spec.ts b/tests/api/api.spec.ts index b29936140..3dbdf98fe 100644 --- a/tests/api/api.spec.ts +++ b/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" }) ]); }); diff --git a/tests/integration/bigquery.spec.ts b/tests/integration/bigquery.spec.ts index 01b03ce23..af3483a7b 100644 --- a/tests/integration/bigquery.spec.ts +++ b/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 diff --git a/tests/integration/bigquery_project/definitions/example_internal_comment_in_query.sqlx b/tests/integration/bigquery_project/definitions/example_internal_comment_in_query.sqlx new file mode 100644 index 000000000..7ff728aef --- /dev/null +++ b/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([]) \ No newline at end of file diff --git a/tests/integration/bigquery_project/definitions/example_trailing_comment_in_operations.sqlx b/tests/integration/bigquery_project/definitions/example_trailing_comment_in_operations.sqlx new file mode 100644 index 000000000..24f79f5f8 --- /dev/null +++ b/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)) +} \ No newline at end of file diff --git a/tests/integration/bigquery_project/definitions/example_trailing_statement_comment_with_operations.sqlx b/tests/integration/bigquery_project/definitions/example_trailing_statement_comment_with_operations.sqlx new file mode 100644 index 000000000..dc47e43f5 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/tools/ts_proto_library.bzl b/tools/ts_proto_library.bzl index c67a2bb45..5d80120e3 100644 --- a/tools/ts_proto_library.bzl +++ b/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