From 2b1ca03e27f60ef6aca1eb280c867296305ea4bc Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Fri, 19 Apr 2024 16:44:53 +0000 Subject: [PATCH 01/12] Feature: dependent assertions as dependencies --- core/actions/assertion.ts | 7 +- core/actions/operation.ts | 32 +++- core/actions/table.ts | 32 +++- core/common.ts | 7 + core/main_test.ts | 335 +++++++++++++++++++++++++++++++++++++ core/session.ts | 80 ++++++++- core/utils.ts | 18 ++ protos/core.proto | 1 + tests/api/projects.spec.ts | 8 +- tests/core/core.spec.ts | 2 +- 10 files changed, 508 insertions(+), 14 deletions(-) diff --git a/core/actions/assertion.ts b/core/actions/assertion.ts index 5f4e2e81c..b47d498d6 100644 --- a/core/actions/assertion.ts +++ b/core/actions/assertion.ts @@ -56,7 +56,8 @@ export const IAssertionConfigProperties = strictKeysOf()([ "name", "schema", "tags", - "type" + "type", + "dependOnDependencyAssertions" ]); /** @@ -151,7 +152,9 @@ export class Assertion extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; newDependencies.forEach(resolvable => { - this.proto.dependencyTargets.push(resolvableAsTarget(resolvable)); + const resolvableTarget = resolvableAsTarget(resolvable); + this.session.actionAssertionMap.set(resolvableTarget, this.proto.target); + this.proto.dependencyTargets.push(resolvableTarget); }); return this; } diff --git a/core/actions/operation.ts b/core/actions/operation.ts index 65106c7fa..066b87818 100644 --- a/core/actions/operation.ts +++ b/core/actions/operation.ts @@ -22,7 +22,8 @@ import { resolveActionsConfigFilename, setNameAndTarget, strictKeysOf, - toResolvable + toResolvable, + isSameTarget } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -59,7 +60,8 @@ export const IIOperationConfigProperties = strictKeysOf()([ "name", "schema", "tags", - "type" + "type", + "dependOnDependencyAssertions" ]); /** @@ -75,6 +77,10 @@ export class Operation extends ActionBuilder { // We delay contextification until the final compile step, so hold these here for now. private contextableQueries: Contextable; + // This is Action level flag. If set to true, we will add assertions from all the depenedncies of + // current action as dependencies. + public dependOnDependencyAssertions: boolean = false; + constructor( session?: Session, config?: dataform.ActionConfig.OperationConfig, @@ -118,6 +124,9 @@ export class Operation extends ActionBuilder { IIOperationConfigProperties, "operation config" ); + if (config.dependOnDependencyAssertions) { + this.setDependOnDependencyAssertions(config.dependOnDependencyAssertions); + } if (config.dependencies) { this.dependencies(config.dependencies); } @@ -156,7 +165,19 @@ export class Operation extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; newDependencies.forEach(resolvable => { - this.proto.dependencyTargets.push(resolvableAsTarget(resolvable)); + let dependencyTarget = resolvableAsTarget(resolvable); + dependencyTarget.includeDependentAssertions = dependencyTarget.includeDependentAssertions===undefined ? this.dependOnDependencyAssertions : dependencyTarget.includeDependentAssertions; + const existingDependencies = this.proto.dependencyTargets.filter(dependency => isSameTarget(dependencyTarget, dependency) && dependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions) + if (existingDependencies.length !== 0){ + this.session.compileError( + `Conflicting "includeDependentAssertions" flag not allowed. Dependency ${dependencyTarget.name} have different value set for this flag.`, + this.proto.fileName, + this.proto.target + ) + return this; + } + + this.proto.dependencyTargets.push(dependencyTarget); }); return this; } @@ -228,6 +249,11 @@ export class Operation extends ActionBuilder { return this; } + public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean){ + this.dependOnDependencyAssertions = dependOnDependencyAssertions; + return this; + } + /** * @hidden */ diff --git a/core/actions/table.ts b/core/actions/table.ts index 537877374..87296dd84 100644 --- a/core/actions/table.ts +++ b/core/actions/table.ts @@ -25,7 +25,8 @@ import { strictKeysOf, tableTypeStringToEnum, toResolvable, - validateQueryString + validateQueryString, + isSameTarget } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -221,7 +222,8 @@ export const ITableConfigProperties = () => "database", "columns", "description", - "materialized" + "materialized", + "dependOnDependencyAssertions" ]); /** @@ -265,6 +267,10 @@ export class Table extends ActionBuilder { private uniqueKeyAssertions: Assertion[] = []; private rowConditionsAssertion: Assertion; + // This is Action level flag. If set to true, we will add assertions from all the depenedncies of + // current action as dependencies. + public dependOnDependencyAssertions: boolean = false; + constructor( session?: Session, tableTypeConfig?: @@ -444,6 +450,9 @@ export class Table extends ActionBuilder { if (config.type) { this.type(config.type); } + if (config.dependOnDependencyAssertions) { + this.setDependOnDependencyAssertions(config.dependOnDependencyAssertions); + } if (config.dependencies) { this.dependencies(config.dependencies); } @@ -553,7 +562,19 @@ export class Table extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; newDependencies.forEach(resolvable => { - this.proto.dependencyTargets.push(resolvableAsTarget(resolvable)); + let dependencyTarget = resolvableAsTarget(resolvable); + dependencyTarget.includeDependentAssertions = dependencyTarget.includeDependentAssertions===undefined ? this.dependOnDependencyAssertions : dependencyTarget.includeDependentAssertions; + const existingDependencies = this.proto.dependencyTargets.filter(dependency => isSameTarget(dependencyTarget, dependency) && dependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions) + if (existingDependencies.length !== 0){ + this.session.compileError( + `Conflicting "includeDependentAssertions" flag not allowed. Dependency ${dependencyTarget.name} have different value set for this flag.`, + this.proto.fileName, + this.proto.target + ) + return this; + } + + this.proto.dependencyTargets.push(dependencyTarget); }); return this; @@ -676,6 +697,11 @@ export class Table extends ActionBuilder { return this; } + public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean){ + this.dependOnDependencyAssertions = dependOnDependencyAssertions; + return this; + } + /** * @hidden */ diff --git a/core/common.ts b/core/common.ts index 106075be9..0292dc7da 100644 --- a/core/common.ts +++ b/core/common.ts @@ -133,6 +133,11 @@ export interface IDependenciesConfig { * dependencies, then it should be set to `true`. */ hermetic?: boolean; + + /** + * If this flag is set to true, assertions depenedent upon any of the dependencies are added as depenedencies as well. + */ + dependOnDependencyAssertions?: boolean; } /** @@ -219,6 +224,8 @@ export interface ITarget { schema?: string; name?: string; + + IncludeDependentAssertions?: boolean; } /** diff --git a/core/main_test.ts b/core/main_test.ts index 1f5fe7049..8a354271e 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -1260,6 +1260,341 @@ actions: expect(result.compile.compiledGraph.operations[0].fileName).deep.equals("table.sql"); }); }); + + suite("assertions as dependencies", () => { + [ + TestConfigs.bigquery, + TestConfigs.bigqueryWithDatasetSuffix, + TestConfigs.bigqueryWithNamePrefix + ].forEach(testConfig => { + test("using only dependOnDependencyAssertions flag", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "table", + dependOnDependencyAssertions: true, + dependencies: ["A"] + } + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3) + }) + + test("using includeDependentAssertions flag in config.dependencies", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "table", + dependencies: [{name: "A", includeDependentAssertions: true}, "C"] + } + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + }) + + test("using includeDependentAssertions flag in ref", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "table", + dependencies: ["A"] + } + select * from \${ref({name: "C", includeDependentAssertions: true})} + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3) + }) + + test("Conflict: dependOnDependencyAssertions True, includeDependentAssertions False", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "table", + dependOnDependencyAssertions: true, + dependencies: ["A"] + } + select * from \${ref({name: "C", includeDependentAssertions: false})} + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + }) + + test("Conflict: using dependOnDependencyAssertions False, includeDependentAssertions True", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "operations", + dependOnDependencyAssertions: false, + dependencies: ["A"] + } + select * from \${ref({name: "C", includeDependentAssertions: true})} + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).equals(3) + }) + + test("Duplicate assertion in config and ref", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "table", + dependencies: ["A_assert", "C"] + } + select * from \${ref({name: "A", includeDependentAssertions: true})} + select * from \${ref({name: "C", includeDependentAssertions: false})} + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + }) + + test("conflicting includeDependentAssertions flag in config and ref", ()=>{ + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + `config { + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + `config { + type: "table", + dependencies: [{name: "A", includeDependentAssertions: false}, {name: "C", includeDependentAssertions: true}] + } + select * from \${ref({name: "A", includeDependentAssertions: true})} + select * from \${ref({name: "C", includeDependentAssertions: false})} + select 1 as btest + `); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + `config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } + } + SELECT 1 as test + } + `); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" flag not allowed. Dependency A have different value set for this flag.`); + }) + + }) + }); + }); function coreExecutionRequestFromPath(projectDir: string): dataform.CoreExecutionRequest { diff --git a/core/session.ts b/core/session.ts index 06d9be58f..9f1b80d64 100644 --- a/core/session.ts +++ b/core/session.ts @@ -9,7 +9,7 @@ import { Notebook } from "df/core/actions/notebook"; import { Operation, OperationContext } from "df/core/actions/operation"; import { ITableConfig, ITableContext, Table, TableContext, TableType } from "df/core/actions/table"; import { Test } from "df/core/actions/test"; -import { Contextable, ICommonContext, Resolvable } from "df/core/common"; +import { Contextable, ICommonContext, ITarget, Resolvable } from "df/core/common"; import { CompilationSql } from "df/core/compilation_sql"; import { targetAsReadableString, targetStringifier } from "df/core/targets"; import * as utils from "df/core/utils"; @@ -50,6 +50,10 @@ export class Session { public indexedActions: ActionIndex; public tests: { [name: string]: Test }; + // This map holds information about what assertions are dependent + // upon a certain action in our actions list. We use this later to resolve dependencies. + public actionAssertionMap = new ActionAssertionMap(); + public graphErrors: dataform.IGraphErrors; constructor( @@ -461,6 +465,12 @@ export class Session { // We found a single matching target, and fully-qualify it if it's a normal dependency. const protoDep = possibleDeps[0].proto; fullyQualifiedDependencies[targetAsReadableString(protoDep.target)] = protoDep.target; + + if (dependency.includeDependentAssertions){ + this.actionAssertionMap.get(dependency).forEach(assertionTarget => + fullyQualifiedDependencies[targetAsReadableString(assertionTarget)] = assertionTarget + ); + } } else { // Too many targets matched the dependency. this.compileError( @@ -806,3 +816,71 @@ class ActionIndex { return this.byName.get(target.name) || []; } } + + +class ActionAssertionMap { + private byName: Map = new Map(); + private bySchemaAndName: Map> = new Map(); + private byDatabaseAndName: Map> = new Map(); + private byDatabaseSchemaAndName: Map< + string, + Map> + > = new Map(); + + public set(actionTarget: ITarget, assertionTarget: dataform.ITarget){ + this.setByNameLevel(this.byName, actionTarget.name, assertionTarget); + + if (!!actionTarget.schema){ + this.setBySchemaLevel(this.bySchemaAndName, actionTarget, assertionTarget); + } + + if (!!actionTarget.database) { + if (!this.byDatabaseAndName.has(actionTarget.database)) { + this.byDatabaseAndName.set(actionTarget.database, new Map()); + } + const forDatabaseNoSchema = this.byDatabaseAndName.get(actionTarget.database); + this.setByNameLevel(forDatabaseNoSchema, actionTarget.name, assertionTarget) + + if (!!actionTarget.schema) { + if (!this.byDatabaseSchemaAndName.has(actionTarget.database)) { + this.byDatabaseSchemaAndName.set(actionTarget.database, new Map()); + } + const forDatabase = this.byDatabaseSchemaAndName.get(actionTarget.database); + this.setBySchemaLevel(forDatabase, actionTarget, assertionTarget); + } + } + } + + public get(target: dataform.ITarget) { + if (!!target.database) { + if (!!target.schema) { + return ( + this.byDatabaseSchemaAndName + .get(target.database) + ?.get(target.schema) + ?.get(target.name) || [] + ); + } + return this.byDatabaseAndName.get(target.database)?.get(target.name) || []; + } + if (!!target.schema) { + return this.bySchemaAndName.get(target.schema)?.get(target.name) || []; + } + return this.byName.get(target.name) || []; + } + + private setByNameLevel(targetMap: Map, name: string, assertionTarget: dataform.ITarget){ + if (!targetMap.has(name)) { + targetMap.set(name, []); + } + targetMap.get(name).push(assertionTarget); + } + + private setBySchemaLevel(targetMap: Map>, actionTarget: ITarget, assertionTarget: dataform.ITarget){ + if (!targetMap.has(actionTarget.schema)) { + targetMap.set(actionTarget.schema, new Map()); + } + const forSchema = targetMap.get(actionTarget.schema); + this.setByNameLevel(forSchema, actionTarget.name, assertionTarget); + } +} diff --git a/core/utils.ts b/core/utils.ts index 18a507043..9ff7997a5 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -299,3 +299,21 @@ export function actionConfigToCompiledGraphTarget( export function resolveActionsConfigFilename(configFilename: string, configPath: string) { return Path.normalize(Path.join(Path.dirName(configPath), configFilename)); } + + +export function isSameTarget(targetA: dataform.ITarget, targetB: dataform.ITarget): boolean { + if (targetA.name !== targetB.name){ + return false + } + if (!!targetA.database || !!targetB.schema){ + if (targetA.database !== targetB.database){ + return false; + } + } + if (!!targetA.schema || !!targetB.schema){ + if(targetA.schema !== targetB.schema){ + return false + } + } + return true +} diff --git a/protos/core.proto b/protos/core.proto index 46ff9cbce..65ed933b9 100644 --- a/protos/core.proto +++ b/protos/core.proto @@ -48,6 +48,7 @@ message Target { string database = 3; string schema = 1; string name = 2; + bool include_dependent_assertions = 4; } message BigQueryOptions { diff --git a/tests/api/projects.spec.ts b/tests/api/projects.spec.ts index 7968a06fa..0ca99ae7e 100644 --- a/tests/api/projects.spec.ts +++ b/tests/api/projects.spec.ts @@ -35,17 +35,17 @@ suite("examples", () => { { fileName: "definitions/has_compile_errors/assertion_with_bigquery.sqlx", message: - 'Unexpected property "bigquery" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "bigquery" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/assertion_with_materialized.sqlx", message: - 'Unexpected property "materialized" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "materialized" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/assertion_with_output.sqlx", message: - 'Unexpected property "hasOutput" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "hasOutput" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/assertion_with_postops.sqlx", @@ -63,7 +63,7 @@ suite("examples", () => { { fileName: "definitions/has_compile_errors/protected_assertion.sqlx", message: - 'Unexpected property "protected" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "protected" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/view_with_incremental.sqlx", diff --git a/tests/core/core.spec.ts b/tests/core/core.spec.ts index fc0d5c17f..bc3990774 100644 --- a/tests/core/core.spec.ts +++ b/tests/core/core.spec.ts @@ -901,7 +901,7 @@ suite("@dataform/core", () => { expect( cGraph.graphErrors.compilationErrors.filter(item => item.message.match( - /Ambiguous Action name: {\"name\":\"a\"}. Did you mean one of: foo.a, bar.a./ + /Ambiguous Action name: {\"name\":\"a\",\"includeDependentAssertions\":false}. Did you mean one of: foo.a, bar.a./ ) ).length ).greaterThan(0); From b5ca52faefc3d630e8493a4825bd5541e4376c51 Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Thu, 25 Apr 2024 17:17:03 +0000 Subject: [PATCH 02/12] Action.yaml support for dependent assertions --- core/actions/notebook.ts | 20 ++- core/actions/operation.ts | 21 +-- core/actions/table.ts | 28 ++-- core/common.ts | 2 +- core/main_test.ts | 291 +++++++++++++++++++++++++++++++++++++- core/utils.ts | 23 ++- protos/configs.proto | 23 +++ 7 files changed, 363 insertions(+), 45 deletions(-) diff --git a/core/actions/notebook.ts b/core/actions/notebook.ts index 15ba9f8ac..bfb50aa6e 100644 --- a/core/actions/notebook.ts +++ b/core/actions/notebook.ts @@ -4,9 +4,11 @@ import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { actionConfigToCompiledGraphTarget, + addDependenciesToActionDependencyTargets, nativeRequire, resolveActionsConfigFilename } from "df/core/utils"; +import { Resolvable } from "df/core/common"; import { dataform } from "df/protos/ts"; /** @@ -18,6 +20,10 @@ export class Notebook extends ActionBuilder { // TODO: make this field private, to enforce proto update logic to happen in this class. public proto: dataform.INotebook = dataform.Notebook.create(); + // This is Action level flag. If set to true, we will add assertions from all the depenedncies of + // current action as dependencies. + public dependOnDependencyAssertions: boolean = false; + constructor( session?: Session, config?: dataform.ActionConfig.NotebookConfig, @@ -35,9 +41,8 @@ export class Notebook extends ActionBuilder { this.proto.target = this.applySessionToTarget(target, config.filename); this.proto.canonicalTarget = this.applySessionCanonicallyToTarget(target); this.proto.tags = config.tags; - this.proto.dependencyTargets = config.dependencyTargets.map(dependencyTarget => - actionConfigToCompiledGraphTarget(dataform.ActionConfig.Target.create(dependencyTarget)) - ); + this.dependOnDependencyAssertions = config.dependOnDependencyAssertions; + this.dependencies(config.dependencyTargets); this.proto.fileName = config.filename; if (config.disabled) { this.proto.disabled = config.disabled; @@ -61,6 +66,15 @@ export class Notebook extends ActionBuilder { return this; } + /** + * @hidden + */ + public dependencies(value: Resolvable | Resolvable[]) { + const newDependencies = Array.isArray(value) ? value : [value]; + newDependencies.forEach(resolvable => addDependenciesToActionDependencyTargets(this, resolvable)); + return this; + } + /** * @hidden */ diff --git a/core/actions/operation.ts b/core/actions/operation.ts index 066b87818..096c00ddc 100644 --- a/core/actions/operation.ts +++ b/core/actions/operation.ts @@ -23,7 +23,7 @@ import { setNameAndTarget, strictKeysOf, toResolvable, - isSameTarget + addDependenciesToActionDependencyTargets } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -111,7 +111,8 @@ export class Operation extends ActionBuilder { tags: config.tags, disabled: config.disabled, hasOutput: config.hasOutput, - description: config.description + description: config.description, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); this.queries(nativeRequire(config.filename).query); @@ -164,21 +165,7 @@ export class Operation extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; - newDependencies.forEach(resolvable => { - let dependencyTarget = resolvableAsTarget(resolvable); - dependencyTarget.includeDependentAssertions = dependencyTarget.includeDependentAssertions===undefined ? this.dependOnDependencyAssertions : dependencyTarget.includeDependentAssertions; - const existingDependencies = this.proto.dependencyTargets.filter(dependency => isSameTarget(dependencyTarget, dependency) && dependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions) - if (existingDependencies.length !== 0){ - this.session.compileError( - `Conflicting "includeDependentAssertions" flag not allowed. Dependency ${dependencyTarget.name} have different value set for this flag.`, - this.proto.fileName, - this.proto.target - ) - return this; - } - - this.proto.dependencyTargets.push(dependencyTarget); - }); + newDependencies.forEach(resolvable => addDependenciesToActionDependencyTargets(this, resolvable)); return this; } diff --git a/core/actions/table.ts b/core/actions/table.ts index 87296dd84..343e8d64c 100644 --- a/core/actions/table.ts +++ b/core/actions/table.ts @@ -26,7 +26,7 @@ import { tableTypeStringToEnum, toResolvable, validateQueryString, - isSameTarget + addDependenciesToActionDependencyTargets, } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -346,7 +346,8 @@ export class Table extends ActionBuilder { tags: config.tags, disabled: config.disabled, description: config.description, - bigquery: bigqueryOptions + bigquery: bigqueryOptions, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); } if (tableType === "view") { @@ -376,7 +377,8 @@ export class Table extends ActionBuilder { materialized: config.materialized, tags: config.tags, description: config.description, - bigquery: bigqueryOptions + bigquery: bigqueryOptions, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); } if (tableType === "incremental") { @@ -428,7 +430,8 @@ export class Table extends ActionBuilder { uniqueKey: config.uniqueKey, tags: config.tags, description: config.description, - bigquery: bigqueryOptions + bigquery: bigqueryOptions, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); } this.query(nativeRequire(tableTypeConfig.filename).query); @@ -561,22 +564,7 @@ export class Table extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; - newDependencies.forEach(resolvable => { - let dependencyTarget = resolvableAsTarget(resolvable); - dependencyTarget.includeDependentAssertions = dependencyTarget.includeDependentAssertions===undefined ? this.dependOnDependencyAssertions : dependencyTarget.includeDependentAssertions; - const existingDependencies = this.proto.dependencyTargets.filter(dependency => isSameTarget(dependencyTarget, dependency) && dependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions) - if (existingDependencies.length !== 0){ - this.session.compileError( - `Conflicting "includeDependentAssertions" flag not allowed. Dependency ${dependencyTarget.name} have different value set for this flag.`, - this.proto.fileName, - this.proto.target - ) - return this; - } - - this.proto.dependencyTargets.push(dependencyTarget); - }); - + newDependencies.forEach(resolvable => addDependenciesToActionDependencyTargets(this, resolvable)); return this; } diff --git a/core/common.ts b/core/common.ts index 0292dc7da..645cfcff2 100644 --- a/core/common.ts +++ b/core/common.ts @@ -225,7 +225,7 @@ export interface ITarget { name?: string; - IncludeDependentAssertions?: boolean; + includeDependentAssertions?: boolean; } /** diff --git a/core/main_test.ts b/core/main_test.ts index 8a354271e..97809bda7 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -1589,12 +1589,297 @@ actions: const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); - expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" flag not allowed. Dependency A have different value set for this flag.`); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" flag not allowed. Dependency A have different values set for this flag.`); }) - + + suite("action configs", () => { + test(`using only dependOnDependencyAssertions flag`, () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + VALID_WORKFLOW_SETTINGS_YAML + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` + actions: + - table: + filename: A.sql + - assertion: + filename: A_assert.sql + dependencyTargets: + - name: A + - view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + - operation: + filename: C.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + - notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + ` + ); + fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(2); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`using includeDependentAssertions flag in config.dependencies`, () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + VALID_WORKFLOW_SETTINGS_YAML + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` + actions: + - table: + filename: A.sql + - assertion: + filename: A_assert.sql + dependencyTargets: + - name: A + - view: + filename: B.sql + dependencyTargets: + - + name: A + includeDependentAssertions: true + - operation: + filename: C.sql + dependencyTargets: + - name: A + includeDependentAssertions: true + - notebook: + filename: notebook.ipynb + dependencyTargets: + - name: A + includeDependentAssertions: true + ` + ); + fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(2); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`Conflict: dependOnDependencyAssertions True, includeDependentAssertions False`, () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + VALID_WORKFLOW_SETTINGS_YAML + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` + actions: + - table: + filename: A.sql + - assertion: + filename: A_assert.sql + dependencyTargets: + - name: A + - view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false + - assertion: + filename: B_assert.sql + dependencyTargets: + - name: B + - operation: + filename: C.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false + - notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false + - name: B + ` + ); + fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(1); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(1); + expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(3); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`Conflict: using dependOnDependencyAssertions False, includeDependentAssertions True`, () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + VALID_WORKFLOW_SETTINGS_YAML + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` + actions: + - table: + filename: A.sql + - assertion: + filename: A_assert.sql + dependencyTargets: + - name: A + - view: + filename: B.sql + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - assertion: + filename: B_assert.sql + dependencyTargets: + - name: B + - operation: + filename: C.sql + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B + - notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B + ` + ); + fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(3); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`conflicting includeDependentAssertions flag in config`, () => { + const projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + VALID_WORKFLOW_SETTINGS_YAML + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` + actions: + - table: + filename: A.sql + - assertion: + filename: A_assert.sql + dependencyTargets: + - name: A + - view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + - assertion: + filename: B_assert.sql + dependencyTargets: + - name: B + - operation: + filename: C.sql + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B + - name: A + includeDependentAssertions: false + - notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: false + - name: B + - name: A + includeDependentAssertions: true + ` + ); + fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" flag not allowed. Dependency A have different values set for this flag.`); + }); + }); }) }); - }); function coreExecutionRequestFromPath(projectDir: string): dataform.CoreExecutionRequest { diff --git a/core/utils.ts b/core/utils.ts index 9ff7997a5..2304f1b72 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -6,10 +6,13 @@ import { Resolvable } from "df/core/common"; import * as Path from "df/core/path"; import { IActionProto, Session } from "df/core/session"; import { dataform } from "df/protos/ts"; +import { Notebook } from "./actions/notebook"; declare var __webpack_require__: any; declare var __non_webpack_require__: any; +type actionsWithDependencies = Table | Operation | Notebook + // This side-steps webpack's require in favour of the real require. export const nativeRequire = typeof __webpack_require__ === "function" ? __non_webpack_require__ : require; @@ -293,6 +296,9 @@ export function actionConfigToCompiledGraphTarget( if (actionConfigTarget.project) { compiledGraphTarget.database = actionConfigTarget.project; } + if (actionConfigTarget.hasOwnProperty("includeDependentAssertions")){ + compiledGraphTarget.includeDependentAssertions = actionConfigTarget.includeDependentAssertions; + } return dataform.Target.create(compiledGraphTarget); } @@ -300,7 +306,6 @@ export function resolveActionsConfigFilename(configFilename: string, configPath: return Path.normalize(Path.join(Path.dirName(configPath), configFilename)); } - export function isSameTarget(targetA: dataform.ITarget, targetB: dataform.ITarget): boolean { if (targetA.name !== targetB.name){ return false @@ -317,3 +322,19 @@ export function isSameTarget(targetA: dataform.ITarget, targetB: dataform.ITarge } return true } + +export function addDependenciesToActionDependencyTargets(action: actionsWithDependencies, resolvable: Resolvable){ + let dependencyTarget = resolvableAsTarget(resolvable); + dependencyTarget.includeDependentAssertions = dependencyTarget.hasOwnProperty("includeDependentAssertions") ? dependencyTarget.includeDependentAssertions : action.dependOnDependencyAssertions; + const existingDependencies = action.proto.dependencyTargets.filter(dependency => isSameTarget(dependencyTarget, dependency) && dependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions) + if (existingDependencies.length !== 0){ + action.session.compileError( + `Conflicting "includeDependentAssertions" flag not allowed. Dependency ${dependencyTarget.name} have different values set for this flag.`, + action.proto.fileName, + action.proto.target + ) + return action; + } + + action.proto.dependencyTargets.push(dependencyTarget); +} \ No newline at end of file diff --git a/protos/configs.proto b/protos/configs.proto index 5b1b870fa..0fd3ff459 100644 --- a/protos/configs.proto +++ b/protos/configs.proto @@ -63,6 +63,9 @@ message ActionConfig { // The name of the action. string name = 4; + + // flag for when we want to add assertions of this dependency in dependency_targets as well + bool include_dependent_assertions = 5; } message ColumnDescriptor { @@ -148,6 +151,10 @@ message ActionConfig { // If the option name contains special characters, encapsulate the name in // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 17; + + // boolean flag when set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool dependOnDependencyAssertions = 18; } message ViewConfig { @@ -208,6 +215,10 @@ message ActionConfig { // If the option name contains special characters, encapsulate the name in // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 14; + + // boolean flag when set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool dependOnDependencyAssertions = 15; } message IncrementalTableConfig { @@ -296,6 +307,10 @@ message ActionConfig { // If the option name contains special characters, encapsulate the name in // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 20; + + // boolean flag when set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool dependOnDependencyAssertions = 21; } message AssertionConfig { @@ -361,6 +376,10 @@ message ActionConfig { // Descriptions of columns within the operation. Can only be set if // hasOutput is true. repeated ColumnDescriptor columns = 10; + + // boolean flag when set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool dependOnDependencyAssertions = 11; } message DeclarationConfig { @@ -407,6 +426,10 @@ message ActionConfig { // Description of the notebook. string description = 8; + // boolean flag when set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool dependOnDependencyAssertions = 9; + // TODO(ekrekr): add a notebook runtime field definition. } From 27da84ee3a44eec3800d52189f2941c49c145515 Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Mon, 29 Apr 2024 13:50:50 +0000 Subject: [PATCH 03/12] Review updates --- core/actions/notebook.ts | 3 +- core/actions/operation.ts | 13 +- core/actions/table.ts | 15 +- core/main_test.ts | 445 ++++++++++++-------------------------- core/session.ts | 16 +- core/utils.ts | 56 ++--- protos/configs.proto | 20 +- 7 files changed, 191 insertions(+), 377 deletions(-) diff --git a/core/actions/notebook.ts b/core/actions/notebook.ts index bfb50aa6e..d02335574 100644 --- a/core/actions/notebook.ts +++ b/core/actions/notebook.ts @@ -20,8 +20,7 @@ export class Notebook extends ActionBuilder { // TODO: make this field private, to enforce proto update logic to happen in this class. public proto: dataform.INotebook = dataform.Notebook.create(); - // This is Action level flag. If set to true, we will add assertions from all the depenedncies of - // current action as dependencies. + // If true, adds the inline assertions of dependencies as direct dependencies for this action. public dependOnDependencyAssertions: boolean = false; constructor( diff --git a/core/actions/operation.ts b/core/actions/operation.ts index 096c00ddc..a6eec20f8 100644 --- a/core/actions/operation.ts +++ b/core/actions/operation.ts @@ -32,10 +32,10 @@ import { dataform } from "df/protos/ts"; */ export interface IOperationConfig extends IActionConfig, - IDependenciesConfig, - IDocumentableConfig, - INamedConfig, - ITargetableConfig { + IDependenciesConfig, + IDocumentableConfig, + INamedConfig, + ITargetableConfig { /** * Declares that this `operations` action creates a dataset which should be referenceable using the `ref` function. * @@ -77,8 +77,7 @@ export class Operation extends ActionBuilder { // We delay contextification until the final compile step, so hold these here for now. private contextableQueries: Contextable; - // This is Action level flag. If set to true, we will add assertions from all the depenedncies of - // current action as dependencies. + // If true, adds the inline assertions of dependencies as direct dependencies for this action. public dependOnDependencyAssertions: boolean = false; constructor( @@ -236,7 +235,7 @@ export class Operation extends ActionBuilder { return this; } - public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean){ + public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean) { this.dependOnDependencyAssertions = dependOnDependencyAssertions; return this; } diff --git a/core/actions/table.ts b/core/actions/table.ts index 343e8d64c..d8768541a 100644 --- a/core/actions/table.ts +++ b/core/actions/table.ts @@ -158,10 +158,10 @@ const ITableAssertionsProperties = () => */ export interface ITableConfig extends IActionConfig, - IDependenciesConfig, - IDocumentableConfig, - INamedConfig, - ITargetableConfig { + IDependenciesConfig, + IDocumentableConfig, + INamedConfig, + ITargetableConfig { /** * The type of the dataset. For more information on how this setting works, check out some of the [guides](guides) * on publishing different types of datasets with Dataform. @@ -267,8 +267,7 @@ export class Table extends ActionBuilder { private uniqueKeyAssertions: Assertion[] = []; private rowConditionsAssertion: Assertion; - // This is Action level flag. If set to true, we will add assertions from all the depenedncies of - // current action as dependencies. + // If true, adds the inline assertions of dependencies as direct dependencies for this action. public dependOnDependencyAssertions: boolean = false; constructor( @@ -685,7 +684,7 @@ export class Table extends ActionBuilder { return this; } - public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean){ + public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean) { this.dependOnDependencyAssertions = dependOnDependencyAssertions; return this; } @@ -754,7 +753,7 @@ export class Table extends ActionBuilder { * @hidden */ export class TableContext implements ITableContext { - constructor(private table: Table, private isIncremental = false) {} + constructor(private table: Table, private isIncremental = false) { } public config(config: ITableConfig) { this.table.config(config); diff --git a/core/main_test.ts b/core/main_test.ts index 97809bda7..4f3405338 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -112,7 +112,7 @@ suite("@dataform/core", ({ afterEach }) => { ].forEach(testConfig => { test( `assertions target context functions with project suffix '${testConfig.projectSuffix}', ` + - `dataset suffix '${testConfig.datasetSuffix}', and name prefix '${testConfig.namePrefix}'`, + `dataset suffix '${testConfig.datasetSuffix}', and name prefix '${testConfig.namePrefix}'`, () => { const projectDir = tmpDirFixture.createNewTmpDir(); fs.writeFileSync( @@ -132,8 +132,8 @@ suite("@dataform/core", ({ afterEach }) => { ).deep.equals([]); expect(asPlainObject(result.compile.compiledGraph.assertions[0].query)).deep.equals( `default-database${testConfig.projectSuffix ? `_suffix` : ""}.` + - `schema${testConfig.datasetSuffix ? `_suffix` : ""}.` + - `${testConfig.namePrefix ? `prefix_` : ""}name` + `schema${testConfig.datasetSuffix ? `_suffix` : ""}.` + + `${testConfig.namePrefix ? `prefix_` : ""}name` ); } ); @@ -1261,88 +1261,67 @@ actions: }); }); - suite("assertions as dependencies", () => { + suite("Assertions as dependencies", ({ beforeEach }) => { [ TestConfigs.bigquery, TestConfigs.bigqueryWithDatasetSuffix, TestConfigs.bigqueryWithNamePrefix ].forEach(testConfig => { - test("using only dependOnDependencyAssertions flag", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + let projectDir: any; + beforeEach("Create temperory dir and files", () => { + projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } + type: "table", + assertions: { + rowConditions: ["test > 1"] } - SELECT 1 as test } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + SELECT 1 as test + } + `); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + type: "assertion", + } + select test from \${ref("A")} where test > 3`); + }); + + test("When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "table", dependOnDependencyAssertions: true, dependencies: ["A"] } select 1 as btest - `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3) + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3) + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", + testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions", + testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert" + ]) }) - test("using includeDependentAssertions flag in config.dependencies", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + test("Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependeny to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "table", dependencies: [{name: "A", includeDependentAssertions: true}, "C"] } select 1 as btest `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), `config { type: "table", assertions: { @@ -1353,42 +1332,27 @@ actions: } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", + testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions", + testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert", + testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C", + ]) }) - test("using includeDependentAssertions flag in ref", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + test("Setting includeDependentAssertions to true in ref, adds assertions from that dependeny to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "table", dependencies: ["A"] } select * from \${ref({name: "C", includeDependentAssertions: true})} select 1 as btest `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), `config { type: "table", assertions: { @@ -1399,35 +1363,19 @@ actions: } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3) + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", + testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C", + testConfig.namePrefix ? `${testConfig.namePrefix}_schema_C_assertions_rowConditions` : "schema_C_assertions_rowConditions" + ]) }) - test("Conflict: dependOnDependencyAssertions True, includeDependentAssertions False", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + test("When dependOnDependencyAssertions=true and includeDependentAssertions=false, the assertions related to dependency should not be added to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "table", dependOnDependencyAssertions: true, dependencies: ["A"] @@ -1435,7 +1383,7 @@ actions: select * from \${ref({name: "C", includeDependentAssertions: false})} select 1 as btest `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), `config { type: "table", assertions: { @@ -1446,35 +1394,20 @@ actions: } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", + testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions", + testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert", + testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C" + ]); }) - test("Conflict: using dependOnDependencyAssertions False, includeDependentAssertions True", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + test("When dependOnDependencyAssertions=false and includeDependentAssertions=true, the assertions related to dependency should be added to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "operations", dependOnDependencyAssertions: false, dependencies: ["A"] @@ -1482,7 +1415,7 @@ actions: select * from \${ref({name: "C", includeDependentAssertions: true})} select 1 as btest `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), `config { type: "table", assertions: { @@ -1493,82 +1426,40 @@ actions: } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).equals(3) + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", + testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C", + testConfig.namePrefix ? `${testConfig.namePrefix}_schema_C_assertions_rowConditions` : "schema_C_assertions_rowConditions", + ]); }) - - test("Duplicate assertion in config and ref", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + + test("If assertions are added using includeDependentAssertions and also by add in dependencies, this duplications should be handle and added only once.", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "table", - dependencies: ["A_assert", "C"] + dependencies: ["A_assert"] } select * from \${ref({name: "A", includeDependentAssertions: true})} - select * from \${ref({name: "C", includeDependentAssertions: false})} select 1 as btest - `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert", + testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", + testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions" + ]); }) - test("conflicting includeDependentAssertions flag in config and ref", ()=>{ - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")); - fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + test("When includeDependentAssertions property in config and ref are set different for same dependency, compilationError is thrown.", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { + `config { type: "table", dependencies: [{name: "A", includeDependentAssertions: false}, {name: "C", includeDependentAssertions: true}] } @@ -1576,7 +1467,7 @@ actions: select * from \${ref({name: "C", includeDependentAssertions: false})} select 1 as btest `); - fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), `config { type: "table", assertions: { @@ -1587,19 +1478,32 @@ actions: } `); - const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); - expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" flag not allowed. Dependency A have different values set for this flag.`); + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" properties are not allowed. Dependency A has different values set for this property.`); }) - suite("action configs", () => { - test(`using only dependOnDependencyAssertions flag`, () => { - const projectDir = tmpDirFixture.createNewTmpDir(); + suite("Action configs", ({ beforeEach }) => { + let projectDir: any; + + beforeEach("Create temp dir and files", () => { + projectDir = tmpDirFixture.createNewTmpDir(); fs.writeFileSync( path.join(projectDir, "workflow_settings.yaml"), - VALID_WORKFLOW_SETTINGS_YAML + dumpYaml(dataform.WorkflowSettings.create(testConfig)) ); - fs.mkdirSync(path.join(projectDir, "definitions")); + fs.mkdirSync(path.join(projectDir, "definitions")) + fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + }); + + test(`When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` @@ -1627,30 +1531,16 @@ actions: - name: A ` ); - fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); - fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); - fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); - fs.writeFileSync( - path.join(projectDir, `definitions/notebook.ipynb`), - EMPTY_NOTEBOOK_CONTENTS - ); - + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(2); expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(2); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); - test(`using includeDependentAssertions flag in config.dependencies`, () => { - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - VALID_WORKFLOW_SETTINGS_YAML - ); - fs.mkdirSync(path.join(projectDir, "definitions")); + test(`Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependeny to dependencyTargets`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` @@ -1679,30 +1569,16 @@ actions: includeDependentAssertions: true ` ); - fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); - fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); - fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); - fs.writeFileSync( - path.join(projectDir, `definitions/notebook.ipynb`), - EMPTY_NOTEBOOK_CONTENTS - ); - + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(2); expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(2); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); - test(`Conflict: dependOnDependencyAssertions True, includeDependentAssertions False`, () => { - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - VALID_WORKFLOW_SETTINGS_YAML - ); - fs.mkdirSync(path.join(projectDir, "definitions")); + test(`When dependOnDependencyAssertions=true and includeDependentAssertions=false, the assertions related to dependency should not be added to dependencyTargets`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` @@ -1738,31 +1614,17 @@ actions: - name: B ` ); - fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); - fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1 as test"); fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); - fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); - fs.writeFileSync( - path.join(projectDir, `definitions/notebook.ipynb`), - EMPTY_NOTEBOOK_CONTENTS - ); - + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(1); expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(1); expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(3); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); - test(`Conflict: using dependOnDependencyAssertions False, includeDependentAssertions True`, () => { - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - VALID_WORKFLOW_SETTINGS_YAML - ); - fs.mkdirSync(path.join(projectDir, "definitions")); + test(`When dependOnDependencyAssertions=false and includeDependentAssertions=true, the assertions related to dependency should be added to dependencyTargets`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` @@ -1799,31 +1661,17 @@ actions: - name: B ` ); - fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); - fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1 as test"); fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); - fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); - fs.writeFileSync( - path.join(projectDir, `definitions/notebook.ipynb`), - EMPTY_NOTEBOOK_CONTENTS - ); - + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - + expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(3); expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(3); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); - test(`conflicting includeDependentAssertions flag in config`, () => { - const projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - VALID_WORKFLOW_SETTINGS_YAML - ); - fs.mkdirSync(path.join(projectDir, "definitions")); + test(`When includeDependentAssertions property are set different for same dependency, compilationError is thrown.`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` @@ -1839,10 +1687,6 @@ actions: dependOnDependencyAssertions: true dependencyTargets: - name: A - - assertion: - filename: B_assert.sql - dependencyTargets: - - name: B - operation: filename: C.sql dependencyTargets: @@ -1851,33 +1695,16 @@ actions: - name: B - name: A includeDependentAssertions: false - - notebook: - filename: notebook.ipynb - dependOnDependencyAssertions: false - dependencyTargets: - - name: A - includeDependentAssertions: false - - name: B - - name: A - includeDependentAssertions: true ` ); - fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); - fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1 as test"); fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); - fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); - fs.writeFileSync( - path.join(projectDir, `definitions/notebook.ipynb`), - EMPTY_NOTEBOOK_CONTENTS - ); - + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - - expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); - expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" flag not allowed. Dependency A have different values set for this flag.`); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(1); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" properties are not allowed. Dependency A has different values set for this property.`); }); - }); + }); }) }); }); diff --git a/core/session.ts b/core/session.ts index 9f1b80d64..713797055 100644 --- a/core/session.ts +++ b/core/session.ts @@ -465,9 +465,9 @@ export class Session { // We found a single matching target, and fully-qualify it if it's a normal dependency. const protoDep = possibleDeps[0].proto; fullyQualifiedDependencies[targetAsReadableString(protoDep.target)] = protoDep.target; - - if (dependency.includeDependentAssertions){ - this.actionAssertionMap.get(dependency).forEach(assertionTarget => + + if (dependency.includeDependentAssertions) { + this.actionAssertionMap.get(dependency).forEach(assertionTarget => fullyQualifiedDependencies[targetAsReadableString(assertionTarget)] = assertionTarget ); } @@ -827,10 +827,10 @@ class ActionAssertionMap { Map> > = new Map(); - public set(actionTarget: ITarget, assertionTarget: dataform.ITarget){ + public set(actionTarget: ITarget, assertionTarget: dataform.ITarget) { this.setByNameLevel(this.byName, actionTarget.name, assertionTarget); - if (!!actionTarget.schema){ + if (!!actionTarget.schema) { this.setBySchemaLevel(this.bySchemaAndName, actionTarget, assertionTarget); } @@ -840,7 +840,7 @@ class ActionAssertionMap { } const forDatabaseNoSchema = this.byDatabaseAndName.get(actionTarget.database); this.setByNameLevel(forDatabaseNoSchema, actionTarget.name, assertionTarget) - + if (!!actionTarget.schema) { if (!this.byDatabaseSchemaAndName.has(actionTarget.database)) { this.byDatabaseSchemaAndName.set(actionTarget.database, new Map()); @@ -869,14 +869,14 @@ class ActionAssertionMap { return this.byName.get(target.name) || []; } - private setByNameLevel(targetMap: Map, name: string, assertionTarget: dataform.ITarget){ + private setByNameLevel(targetMap: Map, name: string, assertionTarget: dataform.ITarget) { if (!targetMap.has(name)) { targetMap.set(name, []); } targetMap.get(name).push(assertionTarget); } - private setBySchemaLevel(targetMap: Map>, actionTarget: ITarget, assertionTarget: dataform.ITarget){ + private setBySchemaLevel(targetMap: Map>, actionTarget: ITarget, assertionTarget: dataform.ITarget) { if (!targetMap.has(actionTarget.schema)) { targetMap.set(actionTarget.schema, new Map()); } diff --git a/core/utils.ts b/core/utils.ts index 2304f1b72..5901e98cb 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -6,7 +6,7 @@ import { Resolvable } from "df/core/common"; import * as Path from "df/core/path"; import { IActionProto, Session } from "df/core/session"; import { dataform } from "df/protos/ts"; -import { Notebook } from "./actions/notebook"; +import { Notebook } from "df/core/actions/notebook"; declare var __webpack_require__: any; declare var __non_webpack_require__: any; @@ -137,8 +137,8 @@ export function ambiguousActionNameMsg(act: Resolvable, allActs: Action[] | stri typeof allActs[0] === "string" ? allActs : (allActs as Array).map( - r => `${r.proto.target.schema}.${r.proto.target.name}` - ); + r => `${r.proto.target.schema}.${r.proto.target.name}` + ); return `Ambiguous Action name: ${stringifyResolvable( act )}. Did you mean one of: ${allActNames.join(", ")}.`; @@ -219,8 +219,7 @@ export function checkExcessProperties( if (extraProperties.length > 0) { reportError( new Error( - `Unexpected property "${extraProperties[0]}"${ - !!name ? ` in ${name}` : "" + `Unexpected property "${extraProperties[0]}"${!!name ? ` in ${name}` : "" }. Supported properties are: ${JSON.stringify(supportedProperties)}` ) ); @@ -296,7 +295,7 @@ export function actionConfigToCompiledGraphTarget( if (actionConfigTarget.project) { compiledGraphTarget.database = actionConfigTarget.project; } - if (actionConfigTarget.hasOwnProperty("includeDependentAssertions")){ + if (actionConfigTarget.hasOwnProperty("includeDependentAssertions")) { compiledGraphTarget.includeDependentAssertions = actionConfigTarget.includeDependentAssertions; } return dataform.Target.create(compiledGraphTarget); @@ -306,35 +305,26 @@ export function resolveActionsConfigFilename(configFilename: string, configPath: return Path.normalize(Path.join(Path.dirName(configPath), configFilename)); } -export function isSameTarget(targetA: dataform.ITarget, targetB: dataform.ITarget): boolean { - if (targetA.name !== targetB.name){ - return false - } - if (!!targetA.database || !!targetB.schema){ - if (targetA.database !== targetB.database){ - return false; - } - } - if (!!targetA.schema || !!targetB.schema){ - if(targetA.schema !== targetB.schema){ - return false - } +export function addDependenciesToActionDependencyTargets(action: actionsWithDependencies, resolvable: Resolvable) { + let dependencyTarget = resolvableAsTarget(resolvable); + if (!dependencyTarget.hasOwnProperty("includeDependentAssertions")) { + // dependency `includeDependentAssertions` takes precedence over the config's `dependOnDependencyAssertions` + dependencyTarget.includeDependentAssertions = action.dependOnDependencyAssertions; } - return true -} -export function addDependenciesToActionDependencyTargets(action: actionsWithDependencies, resolvable: Resolvable){ - let dependencyTarget = resolvableAsTarget(resolvable); - dependencyTarget.includeDependentAssertions = dependencyTarget.hasOwnProperty("includeDependentAssertions") ? dependencyTarget.includeDependentAssertions : action.dependOnDependencyAssertions; - const existingDependencies = action.proto.dependencyTargets.filter(dependency => isSameTarget(dependencyTarget, dependency) && dependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions) - if (existingDependencies.length !== 0){ + // check if same dependency already exist in this action but with opposite value for includeDependentAssertions + const conflictingIncludeDependentAssertions = + action.proto.dependencyTargets.filter(existingDependency => + action.session.compilationSql().resolveTarget(existingDependency) === action.session.compilationSql().resolveTarget(dependencyTarget) + && existingDependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions + ); + if (conflictingIncludeDependentAssertions.length !== 0) { action.session.compileError( - `Conflicting "includeDependentAssertions" flag not allowed. Dependency ${dependencyTarget.name} have different values set for this flag.`, - action.proto.fileName, - action.proto.target - ) - return action; + `Conflicting "includeDependentAssertions" properties are not allowed. Dependency ${dependencyTarget.name} has different values set for this property.`, + action.proto.fileName, + action.proto.target + ) + return action; } - action.proto.dependencyTargets.push(dependencyTarget); -} \ No newline at end of file +} diff --git a/protos/configs.proto b/protos/configs.proto index 0fd3ff459..723917a16 100644 --- a/protos/configs.proto +++ b/protos/configs.proto @@ -152,9 +152,9 @@ message ActionConfig { // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 17; - // boolean flag when set to true, assertions dependent upon any dependency will + // When set to true, assertions dependent upon any dependency will // be add as dedpendency to this action - bool dependOnDependencyAssertions = 18; + bool depend_on_dependency_assertions = 18; } message ViewConfig { @@ -216,9 +216,9 @@ message ActionConfig { // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 14; - // boolean flag when set to true, assertions dependent upon any dependency will + // When set to true, assertions dependent upon any dependency will // be add as dedpendency to this action - bool dependOnDependencyAssertions = 15; + bool depend_on_dependency_assertions = 15; } message IncrementalTableConfig { @@ -308,9 +308,9 @@ message ActionConfig { // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 20; - // boolean flag when set to true, assertions dependent upon any dependency will + // When set to true, assertions dependent upon any dependency will // be add as dedpendency to this action - bool dependOnDependencyAssertions = 21; + bool depend_on_dependency_assertions = 21; } message AssertionConfig { @@ -377,9 +377,9 @@ message ActionConfig { // hasOutput is true. repeated ColumnDescriptor columns = 10; - // boolean flag when set to true, assertions dependent upon any dependency will + // When set to true, assertions dependent upon any dependency will // be add as dedpendency to this action - bool dependOnDependencyAssertions = 11; + bool depend_on_dependency_assertions = 11; } message DeclarationConfig { @@ -426,9 +426,9 @@ message ActionConfig { // Description of the notebook. string description = 8; - // boolean flag when set to true, assertions dependent upon any dependency will + // When set to true, assertions dependent upon any dependency will // be add as dedpendency to this action - bool dependOnDependencyAssertions = 9; + bool depend_on_dependency_assertions = 9; // TODO(ekrekr): add a notebook runtime field definition. } From 4dff5732d5b90ce002401d86df5ef911e760eecd Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Tue, 30 Apr 2024 12:55:00 +0000 Subject: [PATCH 04/12] spell check and ActionMap --- core/actions/assertion.ts | 2 +- core/main_test.ts | 16 +++---- core/session.ts | 95 +++++++-------------------------------- core/utils.ts | 2 +- 4 files changed, 25 insertions(+), 90 deletions(-) diff --git a/core/actions/assertion.ts b/core/actions/assertion.ts index b47d498d6..f9580533f 100644 --- a/core/actions/assertion.ts +++ b/core/actions/assertion.ts @@ -153,7 +153,7 @@ export class Assertion extends ActionBuilder { const newDependencies = Array.isArray(value) ? value : [value]; newDependencies.forEach(resolvable => { const resolvableTarget = resolvableAsTarget(resolvable); - this.session.actionAssertionMap.set(resolvableTarget, this.proto.target); + this.session.actionAssertionMap.set(resolvableTarget, this); this.proto.dependencyTargets.push(resolvableTarget); }); return this; diff --git a/core/main_test.ts b/core/main_test.ts index 4f3405338..af43a9777 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -1268,7 +1268,7 @@ actions: TestConfigs.bigqueryWithNamePrefix ].forEach(testConfig => { let projectDir: any; - beforeEach("Create temperory dir and files", () => { + beforeEach("Create temporary dir and files", () => { projectDir = tmpDirFixture.createNewTmpDir(); fs.writeFileSync( path.join(projectDir, "workflow_settings.yaml"), @@ -1313,7 +1313,7 @@ actions: ]) }) - test("Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependeny to dependencyTargets", () => { + test("Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependency to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), `config { type: "table", @@ -1343,7 +1343,7 @@ actions: ]) }) - test("Setting includeDependentAssertions to true in ref, adds assertions from that dependeny to dependencyTargets", () => { + test("Setting includeDependentAssertions to true in ref, adds assertions from that dependency to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), `config { type: "table", @@ -1436,7 +1436,7 @@ actions: ]); }) - test("If assertions are added using includeDependentAssertions and also by add in dependencies, this duplications should be handle and added only once.", () => { + test("Assertions added through includeDependentAssertions and explicitly listed in dependencies are deduplicated.", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), `config { type: "table", @@ -1457,7 +1457,7 @@ actions: ]); }) - test("When includeDependentAssertions property in config and ref are set different for same dependency, compilationError is thrown.", () => { + test("When includeDependentAssertions property in config and ref are set differently for the same dependency, compilation error is thrown.", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), `config { type: "table", @@ -1486,7 +1486,7 @@ actions: suite("Action configs", ({ beforeEach }) => { let projectDir: any; - beforeEach("Create temp dir and files", () => { + beforeEach("Create temporary dir and files", () => { projectDir = tmpDirFixture.createNewTmpDir(); fs.writeFileSync( path.join(projectDir, "workflow_settings.yaml"), @@ -1540,7 +1540,7 @@ actions: expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); - test(`Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependeny to dependencyTargets`, () => { + test(`Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependency to dependencyTargets`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` @@ -1671,7 +1671,7 @@ actions: expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); - test(`When includeDependentAssertions property are set different for same dependency, compilationError is thrown.`, () => { + test(`When includeDependentAssertions property in config and ref are set differently for the same dependency, compilation error is thrown.`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` diff --git a/core/session.ts b/core/session.ts index 713797055..96c4d8de8 100644 --- a/core/session.ts +++ b/core/session.ts @@ -47,12 +47,12 @@ export class Session { public canonicalConfig: dataform.IProjectConfig; public actions: Action[]; - public indexedActions: ActionIndex; + public indexedActions: ActionMap; public tests: { [name: string]: Test }; // This map holds information about what assertions are dependent // upon a certain action in our actions list. We use this later to resolve dependencies. - public actionAssertionMap = new ActionAssertionMap(); + public actionAssertionMap = new ActionMap([]); public graphErrors: dataform.IGraphErrors; @@ -325,7 +325,7 @@ export class Session { } public compile(): dataform.CompiledGraph { - this.indexedActions = new ActionIndex(this.actions); + this.indexedActions = new ActionMap(this.actions); if ( (this.config.warehouse === "bigquery" || this.config.warehouse === "") && @@ -467,8 +467,8 @@ export class Session { fullyQualifiedDependencies[targetAsReadableString(protoDep.target)] = protoDep.target; if (dependency.includeDependentAssertions) { - this.actionAssertionMap.get(dependency).forEach(assertionTarget => - fullyQualifiedDependencies[targetAsReadableString(assertionTarget)] = assertionTarget + this.actionAssertionMap.find(dependency).forEach(assertion => + fullyQualifiedDependencies[targetAsReadableString(assertion.proto.target)] = assertion.proto.target ); } } else { @@ -747,87 +747,22 @@ function objectExistsOrIsNonEmpty(prop: any): boolean { ); } -class ActionIndex { - private readonly byName: Map = new Map(); - private readonly bySchemaAndName: Map> = new Map(); - private readonly byDatabaseAndName: Map> = new Map(); - private readonly byDatabaseSchemaAndName: Map< +class ActionMap { + private byName: Map = new Map(); + private bySchemaAndName: Map> = new Map(); + private byDatabaseAndName: Map> = new Map(); + private byDatabaseSchemaAndName: Map< string, Map> > = new Map(); public constructor(actions: Action[]) { for (const action of actions) { - if (!this.byName.has(action.proto.target.name)) { - this.byName.set(action.proto.target.name, []); - } - this.byName.get(action.proto.target.name).push(action); - - if (!this.bySchemaAndName.has(action.proto.target.schema)) { - this.bySchemaAndName.set(action.proto.target.schema, new Map()); - } - const forSchema = this.bySchemaAndName.get(action.proto.target.schema); - if (!forSchema.has(action.proto.target.name)) { - forSchema.set(action.proto.target.name, []); - } - forSchema.get(action.proto.target.name).push(action); - - if (!!action.proto.target.database) { - if (!this.byDatabaseAndName.has(action.proto.target.database)) { - this.byDatabaseAndName.set(action.proto.target.database, new Map()); - } - const forDatabaseNoSchema = this.byDatabaseAndName.get(action.proto.target.database); - if (!forDatabaseNoSchema.has(action.proto.target.name)) { - forDatabaseNoSchema.set(action.proto.target.name, []); - } - forDatabaseNoSchema.get(action.proto.target.name).push(action); - - if (!this.byDatabaseSchemaAndName.has(action.proto.target.database)) { - this.byDatabaseSchemaAndName.set(action.proto.target.database, new Map()); - } - const forDatabase = this.byDatabaseSchemaAndName.get(action.proto.target.database); - if (!forDatabase.has(action.proto.target.schema)) { - forDatabase.set(action.proto.target.schema, new Map()); - } - const forDatabaseAndSchema = forDatabase.get(action.proto.target.schema); - if (!forDatabaseAndSchema.has(action.proto.target.name)) { - forDatabaseAndSchema.set(action.proto.target.name, []); - } - forDatabaseAndSchema.get(action.proto.target.name).push(action); - } - } - } - - public find(target: dataform.ITarget) { - if (!!target.database) { - if (!!target.schema) { - return ( - this.byDatabaseSchemaAndName - .get(target.database) - ?.get(target.schema) - ?.get(target.name) || [] - ); - } - return this.byDatabaseAndName.get(target.database)?.get(target.name) || []; + this.set(action.proto.target, action) } - if (!!target.schema) { - return this.bySchemaAndName.get(target.schema)?.get(target.name) || []; - } - return this.byName.get(target.name) || []; } -} - -class ActionAssertionMap { - private byName: Map = new Map(); - private bySchemaAndName: Map> = new Map(); - private byDatabaseAndName: Map> = new Map(); - private byDatabaseSchemaAndName: Map< - string, - Map> - > = new Map(); - - public set(actionTarget: ITarget, assertionTarget: dataform.ITarget) { + public set(actionTarget: ITarget, assertionTarget: Action) { this.setByNameLevel(this.byName, actionTarget.name, assertionTarget); if (!!actionTarget.schema) { @@ -851,7 +786,7 @@ class ActionAssertionMap { } } - public get(target: dataform.ITarget) { + public find(target: dataform.ITarget) { if (!!target.database) { if (!!target.schema) { return ( @@ -869,14 +804,14 @@ class ActionAssertionMap { return this.byName.get(target.name) || []; } - private setByNameLevel(targetMap: Map, name: string, assertionTarget: dataform.ITarget) { + private setByNameLevel(targetMap: Map, name: string, assertionTarget: Action) { if (!targetMap.has(name)) { targetMap.set(name, []); } targetMap.get(name).push(assertionTarget); } - private setBySchemaLevel(targetMap: Map>, actionTarget: ITarget, assertionTarget: dataform.ITarget) { + private setBySchemaLevel(targetMap: Map>, actionTarget: ITarget, assertionTarget: Action) { if (!targetMap.has(actionTarget.schema)) { targetMap.set(actionTarget.schema, new Map()); } diff --git a/core/utils.ts b/core/utils.ts index 5901e98cb..68784e217 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -1,12 +1,12 @@ import { Action } from "df/core/actions"; import { Assertion } from "df/core/actions/assertion"; +import { Notebook } from "df/core/actions/notebook"; import { Operation } from "df/core/actions/operation"; import { Table } from "df/core/actions/table"; import { Resolvable } from "df/core/common"; import * as Path from "df/core/path"; import { IActionProto, Session } from "df/core/session"; import { dataform } from "df/protos/ts"; -import { Notebook } from "df/core/actions/notebook"; declare var __webpack_require__: any; declare var __non_webpack_require__: any; From 97d71d41a2da30432b90b38fc5099a593ae9dc07 Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Tue, 30 Apr 2024 13:25:15 +0000 Subject: [PATCH 05/12] dependency duplication check O(n) --- core/actions/index.ts | 1 + core/utils.ts | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/core/actions/index.ts b/core/actions/index.ts index 9afe2cc6d..b7de08086 100644 --- a/core/actions/index.ts +++ b/core/actions/index.ts @@ -23,6 +23,7 @@ export type SqlxConfig = ( export abstract class ActionBuilder { public session: Session; + public includeAssertionsForDependencyMap: Map = new Map(); constructor(session?: Session) { this.session = session; diff --git a/core/utils.ts b/core/utils.ts index 68784e217..edad8dc16 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -313,18 +313,18 @@ export function addDependenciesToActionDependencyTargets(action: actionsWithDepe } // check if same dependency already exist in this action but with opposite value for includeDependentAssertions - const conflictingIncludeDependentAssertions = - action.proto.dependencyTargets.filter(existingDependency => - action.session.compilationSql().resolveTarget(existingDependency) === action.session.compilationSql().resolveTarget(dependencyTarget) - && existingDependency.includeDependentAssertions !== dependencyTarget.includeDependentAssertions - ); - if (conflictingIncludeDependentAssertions.length !== 0) { - action.session.compileError( - `Conflicting "includeDependentAssertions" properties are not allowed. Dependency ${dependencyTarget.name} has different values set for this property.`, - action.proto.fileName, - action.proto.target - ) - return action; + const dependencyTargetString = action.session.compilationSql().resolveTarget(dependencyTarget) + + if (action.includeAssertionsForDependencyMap.has(dependencyTargetString)){ + if (action.includeAssertionsForDependencyMap.get(dependencyTargetString) !== dependencyTarget.includeDependentAssertions){ + action.session.compileError( + `Conflicting "includeDependentAssertions" properties are not allowed. Dependency ${dependencyTarget.name} has different values set for this property.`, + action.proto.fileName, + action.proto.target + ) + return action; + } } action.proto.dependencyTargets.push(dependencyTarget); + action.includeAssertionsForDependencyMap.set(dependencyTargetString, dependencyTarget.includeDependentAssertions) } From 3aa99a9ee18afd24c04f4732f1b4a05b9f11bb20 Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Tue, 30 Apr 2024 13:38:06 +0000 Subject: [PATCH 06/12] Formating --- core/utils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/utils.ts b/core/utils.ts index edad8dc16..442f00d07 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -315,8 +315,8 @@ export function addDependenciesToActionDependencyTargets(action: actionsWithDepe // check if same dependency already exist in this action but with opposite value for includeDependentAssertions const dependencyTargetString = action.session.compilationSql().resolveTarget(dependencyTarget) - if (action.includeAssertionsForDependencyMap.has(dependencyTargetString)){ - if (action.includeAssertionsForDependencyMap.get(dependencyTargetString) !== dependencyTarget.includeDependentAssertions){ + if (action.includeAssertionsForDependencyMap.has(dependencyTargetString)) { + if (action.includeAssertionsForDependencyMap.get(dependencyTargetString) !== dependencyTarget.includeDependentAssertions) { action.session.compileError( `Conflicting "includeDependentAssertions" properties are not allowed. Dependency ${dependencyTarget.name} has different values set for this property.`, action.proto.fileName, From 58d95e3816ec6ecfe63873f523b4bac6180bc7e0 Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Tue, 30 Apr 2024 15:54:13 +0000 Subject: [PATCH 07/12] includeAssertionsForDependencyMap changed to includeAssertionsForDependency --- core/actions/index.ts | 2 +- core/utils.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/actions/index.ts b/core/actions/index.ts index b7de08086..b2c542b27 100644 --- a/core/actions/index.ts +++ b/core/actions/index.ts @@ -23,7 +23,7 @@ export type SqlxConfig = ( export abstract class ActionBuilder { public session: Session; - public includeAssertionsForDependencyMap: Map = new Map(); + public includeAssertionsForDependency: Map = new Map(); constructor(session?: Session) { this.session = session; diff --git a/core/utils.ts b/core/utils.ts index 442f00d07..8e1f3cc99 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -315,8 +315,8 @@ export function addDependenciesToActionDependencyTargets(action: actionsWithDepe // check if same dependency already exist in this action but with opposite value for includeDependentAssertions const dependencyTargetString = action.session.compilationSql().resolveTarget(dependencyTarget) - if (action.includeAssertionsForDependencyMap.has(dependencyTargetString)) { - if (action.includeAssertionsForDependencyMap.get(dependencyTargetString) !== dependencyTarget.includeDependentAssertions) { + if (action.includeAssertionsForDependency.has(dependencyTargetString)) { + if (action.includeAssertionsForDependency.get(dependencyTargetString) !== dependencyTarget.includeDependentAssertions) { action.session.compileError( `Conflicting "includeDependentAssertions" properties are not allowed. Dependency ${dependencyTarget.name} has different values set for this property.`, action.proto.fileName, @@ -326,5 +326,5 @@ export function addDependenciesToActionDependencyTargets(action: actionsWithDepe } } action.proto.dependencyTargets.push(dependencyTarget); - action.includeAssertionsForDependencyMap.set(dependencyTargetString, dependencyTarget.includeDependentAssertions) + action.includeAssertionsForDependency.set(dependencyTargetString, dependencyTarget.includeDependentAssertions) } From d029341a31b1348b04c0c18a6b1e64c36ea119dc Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Thu, 2 May 2024 14:22:36 +0000 Subject: [PATCH 08/12] review fixes --- core/main_test.ts | 565 +++++++++++++++++++++++----------------------- version.bzl | 2 +- 2 files changed, 282 insertions(+), 285 deletions(-) diff --git a/core/main_test.ts b/core/main_test.ts index af43a9777..20ace35ac 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -1276,207 +1276,201 @@ actions: ); fs.mkdirSync(path.join(projectDir, "definitions")); fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); + ` +config { + type: "table", + assertions: {rowConditions: ["test > 1"]}} + SELECT 1 as test`); fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), - `config { - type: "assertion", - } - select test from \${ref("A")} where test > 3`); + ` +config { + type: "assertion", +} +select test from \${ref("A")} where test > 3`); }); test("When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "table", - dependOnDependencyAssertions: true, - dependencies: ["A"] - } - select 1 as btest - `); +`config { + type: "table", + dependOnDependencyAssertions: true, + dependencies: ["A"] +} +select 1 as btest +`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3) - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ - testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", - testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions", - testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert" + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3) + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + prefixAdjustedName(testConfig.namePrefix, "A_assert"), ]) }) test("Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependency to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "table", - dependencies: [{name: "A", includeDependentAssertions: true}, "C"] - } - select 1 as btest - `); + ` +config { + type: "table", + dependencies: [{name: "A", includeDependentAssertions: true}, "C"] +} +select 1 as btest`); fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); + ` +config { +type: "table", + assertions: { + rowConditions: ["test > 1"] +} +} +SELECT 1 as test +}`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ - testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", - testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions", - testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert", - testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C", + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(4) + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + prefixAdjustedName(testConfig.namePrefix, "C"), ]) }) test("Setting includeDependentAssertions to true in ref, adds assertions from that dependency to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "table", - dependencies: ["A"] - } - select * from \${ref({name: "C", includeDependentAssertions: true})} - select 1 as btest - `); + ` +config { + type: "table", + dependencies: ["A"] +} +select * from \${ref({name: "C", includeDependentAssertions: true})} +select 1 as btest`); fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ - testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", - testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C", - testConfig.namePrefix ? `${testConfig.namePrefix}_schema_C_assertions_rowConditions` : "schema_C_assertions_rowConditions" + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "C"), + prefixAdjustedName(testConfig.namePrefix, "schema_C_assertions_rowConditions"), ]) }) test("When dependOnDependencyAssertions=true and includeDependentAssertions=false, the assertions related to dependency should not be added to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "table", - dependOnDependencyAssertions: true, - dependencies: ["A"] - } - select * from \${ref({name: "C", includeDependentAssertions: false})} - select 1 as btest - `); + ` +config { + type: "table", + dependOnDependencyAssertions: true, + dependencies: ["A"] +} +select * from \${ref({name: "C", includeDependentAssertions: false})} +select 1 as btest`); fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(4) - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ - testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", - testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions", - testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert", - testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C" + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(4) + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + prefixAdjustedName(testConfig.namePrefix, "C"), ]); }) test("When dependOnDependencyAssertions=false and includeDependentAssertions=true, the assertions related to dependency should be added to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "operations", - dependOnDependencyAssertions: false, - dependencies: ["A"] - } - select * from \${ref({name: "C", includeDependentAssertions: true})} - select 1 as btest - `); + ` +config { + type: "operations", + dependOnDependencyAssertions: false, + dependencies: ["A"] +} +select * from \${ref({name: "C", includeDependentAssertions: true})} +select 1 as btest`); fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).equals(3); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ - testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", - testConfig.namePrefix ? `${testConfig.namePrefix}_C` : "C", - testConfig.namePrefix ? `${testConfig.namePrefix}_schema_C_assertions_rowConditions` : "schema_C_assertions_rowConditions", + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "C"), + prefixAdjustedName(testConfig.namePrefix, "schema_C_assertions_rowConditions"), ]); }) test("Assertions added through includeDependentAssertions and explicitly listed in dependencies are deduplicated.", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "table", - dependencies: ["A_assert"] - } - select * from \${ref({name: "A", includeDependentAssertions: true})} - select 1 as btest - `); + ` +config { + type: "table", + dependencies: ["A_assert"] +} +select * from \${ref({name: "A", includeDependentAssertions: true})} +select 1 as btest`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).equals(3); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ - testConfig.namePrefix ? `${testConfig.namePrefix}_A_assert` : "A_assert", - testConfig.namePrefix ? `${testConfig.namePrefix}_A` : "A", - testConfig.namePrefix ? `${testConfig.namePrefix}_schema_A_assertions_rowConditions` : "schema_A_assertions_rowConditions" + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), ]); }) test("When includeDependentAssertions property in config and ref are set differently for the same dependency, compilation error is thrown.", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - `config { - type: "table", - dependencies: [{name: "A", includeDependentAssertions: false}, {name: "C", includeDependentAssertions: true}] - } - select * from \${ref({name: "A", includeDependentAssertions: true})} - select * from \${ref({name: "C", includeDependentAssertions: false})} - select 1 as btest - `); + ` +config { + type: "table", + dependencies: [{name: "A", includeDependentAssertions: false}, {name: "C", includeDependentAssertions: true}] +} +select * from \${ref({name: "A", includeDependentAssertions: true})} +select * from \${ref({name: "C", includeDependentAssertions: false})} +select 1 as btest`); fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - `config { - type: "table", - assertions: { - rowConditions: ["test > 1"] - } - } - SELECT 1 as test - } - `); + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test +}`); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); @@ -1507,36 +1501,36 @@ actions: fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` - actions: - - table: - filename: A.sql - - assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - - view: - filename: B.sql - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - - operation: - filename: C.sql - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - - notebook: - filename: notebook.ipynb - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - ` +actions: +- table: + filename: A.sql +- assertion: + filename: A_assert.sql + dependencyTargets: + - name: A +- view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +- operation: + filename: C.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +- notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +` ); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(2); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1544,37 +1538,36 @@ actions: fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` - actions: - - table: - filename: A.sql - - assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - - view: - filename: B.sql - dependencyTargets: - - - name: A - includeDependentAssertions: true - - operation: - filename: C.sql - dependencyTargets: - - name: A - includeDependentAssertions: true - - notebook: - filename: notebook.ipynb - dependencyTargets: - - name: A - includeDependentAssertions: true - ` +actions: +- table: + filename: A.sql +- assertion: + filename: A_assert.sql + dependencyTargets: + - name: A +- view: + filename: B.sql + dependencyTargets: + - name: A + includeDependentAssertions: true +- operation: + filename: C.sql + dependencyTargets: + - name: A + includeDependentAssertions: true +- notebook: + filename: notebook.ipynb + dependencyTargets: + - name: A + includeDependentAssertions: true +` ); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(2); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1582,45 +1575,45 @@ actions: fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` - actions: - - table: - filename: A.sql - - assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - - view: - filename: B.sql - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - includeDependentAssertions: false - - assertion: - filename: B_assert.sql - dependencyTargets: - - name: B - - operation: - filename: C.sql - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - includeDependentAssertions: false - - notebook: - filename: notebook.ipynb - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - includeDependentAssertions: false - - name: B - ` +actions: +- table: + filename: A.sql +- assertion: + filename: A_assert.sql + dependencyTargets: + - name: A +- view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false +- assertion: + filename: B_assert.sql + dependencyTargets: + - name: B +- operation: + filename: C.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false +- notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false + - name: B +` ); fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(1); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(1); - expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(1); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(1); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1628,46 +1621,46 @@ actions: fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` - actions: - - table: - filename: A.sql - - assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - - view: - filename: B.sql - dependOnDependencyAssertions: false - dependencyTargets: - - name: A - includeDependentAssertions: true - - assertion: - filename: B_assert.sql - dependencyTargets: - - name: B - - operation: - filename: C.sql - dependOnDependencyAssertions: false - dependencyTargets: - - name: A - includeDependentAssertions: true - - name: B - - notebook: - filename: notebook.ipynb - dependOnDependencyAssertions: false - dependencyTargets: - - name: A - includeDependentAssertions: true - - name: B - ` +actions: +- table: + filename: A.sql +- assertion: + filename: A_assert.sql + dependencyTargets: + - name: A +- view: + filename: B.sql + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true +- assertion: + filename: B_assert.sql + dependencyTargets: + - name: B +- operation: + filename: C.sql + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B +- notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B +` ); fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(asPlainObject(result.compile.compiledGraph.operations[0].dependencyTargets.length)).deep.equals(3); - expect(asPlainObject(result.compile.compiledGraph.tables[1].dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.notebooks[0].dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1675,27 +1668,27 @@ actions: fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` - actions: - - table: - filename: A.sql - - assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - - view: - filename: B.sql - dependOnDependencyAssertions: true - dependencyTargets: - - name: A - - operation: - filename: C.sql - dependencyTargets: - - name: A - includeDependentAssertions: true - - name: B - - name: A - includeDependentAssertions: false - ` +actions: +- table: + filename: A.sql +- assertion: + filename: A_assert.sql + dependencyTargets: + - name: A +- view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +- operation: + filename: C.sql + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B + - name: A + includeDependentAssertions: false +` ); fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); @@ -1768,3 +1761,7 @@ function walkDirectoryForFilenames(projectDir: string, relativePath: string = "" }); return paths.map(filename => path.join(relativePath, filename)); } + +function prefixAdjustedName(prefix: string | undefined, name: string){ + return prefix ? `${prefix}_${name}` : name +} diff --git a/version.bzl b/version.bzl index e65a35d23..c6080b9de 100644 --- a/version.bzl +++ b/version.bzl @@ -1,3 +1,3 @@ # NOTE: If you change the format of this line, you must change the bash command # in /scripts/publish to extract the version string correctly. -DF_VERSION = "3.0.0-beta.4" +DF_VERSION = "3.0.0-beta.5" From 4581c2826b789e35990bb2993a33cbf4e69e487b Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Mon, 6 May 2024 12:24:27 +0000 Subject: [PATCH 09/12] Lint fixes --- core/actions/notebook.ts | 2 +- core/actions/operation.ts | 8 ++++---- core/actions/table.ts | 8 ++++---- core/main_test.ts | 8 +++----- core/utils.ts | 2 +- 5 files changed, 13 insertions(+), 15 deletions(-) diff --git a/core/actions/notebook.ts b/core/actions/notebook.ts index d02335574..7cefc2d7a 100644 --- a/core/actions/notebook.ts +++ b/core/actions/notebook.ts @@ -1,5 +1,6 @@ import { verifyObjectMatchesProto } from "df/common/protos"; import { ActionBuilder } from "df/core/actions"; +import { Resolvable } from "df/core/common"; import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { @@ -8,7 +9,6 @@ import { nativeRequire, resolveActionsConfigFilename } from "df/core/utils"; -import { Resolvable } from "df/core/common"; import { dataform } from "df/protos/ts"; /** diff --git a/core/actions/operation.ts b/core/actions/operation.ts index a6eec20f8..da2507d0c 100644 --- a/core/actions/operation.ts +++ b/core/actions/operation.ts @@ -16,6 +16,7 @@ import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { actionConfigToCompiledGraphTarget, + addDependenciesToActionDependencyTargets, checkExcessProperties, nativeRequire, resolvableAsTarget, @@ -23,7 +24,6 @@ import { setNameAndTarget, strictKeysOf, toResolvable, - addDependenciesToActionDependencyTargets } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -74,12 +74,12 @@ export class Operation extends ActionBuilder { // Hold a reference to the Session instance. public session: Session; - // We delay contextification until the final compile step, so hold these here for now. - private contextableQueries: Contextable; - // If true, adds the inline assertions of dependencies as direct dependencies for this action. public dependOnDependencyAssertions: boolean = false; + // We delay contextification until the final compile step, so hold these here for now. + private contextableQueries: Contextable; + constructor( session?: Session, config?: dataform.ActionConfig.OperationConfig, diff --git a/core/actions/table.ts b/core/actions/table.ts index d8768541a..60e2e02c7 100644 --- a/core/actions/table.ts +++ b/core/actions/table.ts @@ -17,6 +17,7 @@ import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { actionConfigToCompiledGraphTarget, + addDependenciesToActionDependencyTargets, checkExcessProperties, nativeRequire, resolvableAsTarget, @@ -26,7 +27,6 @@ import { tableTypeStringToEnum, toResolvable, validateQueryString, - addDependenciesToActionDependencyTargets, } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -258,6 +258,9 @@ export class Table extends ActionBuilder { // Hold a reference to the Session instance. public session: Session; + // If true, adds the inline assertions of dependencies as direct dependencies for this action. + public dependOnDependencyAssertions: boolean = false; + // We delay contextification until the final compile step, so hold these here for now. public contextableQuery: Contextable; private contextableWhere: Contextable; @@ -267,9 +270,6 @@ export class Table extends ActionBuilder { private uniqueKeyAssertions: Assertion[] = []; private rowConditionsAssertion: Assertion; - // If true, adds the inline assertions of dependencies as direct dependencies for this action. - public dependOnDependencyAssertions: boolean = false; - constructor( session?: Session, tableTypeConfig?: diff --git a/core/main_test.ts b/core/main_test.ts index 20ace35ac..553e7bdf0 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -1312,14 +1312,14 @@ select 1 as btest test("Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependency to dependencyTargets", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), - ` + ` config { type: "table", dependencies: [{name: "A", includeDependentAssertions: true}, "C"] } select 1 as btest`); fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), - ` + ` config { type: "table", assertions: { @@ -1478,8 +1478,6 @@ SELECT 1 as test }) suite("Action configs", ({ beforeEach }) => { - let projectDir: any; - beforeEach("Create temporary dir and files", () => { projectDir = tmpDirFixture.createNewTmpDir(); fs.writeFileSync( @@ -1762,6 +1760,6 @@ function walkDirectoryForFilenames(projectDir: string, relativePath: string = "" return paths.map(filename => path.join(relativePath, filename)); } -function prefixAdjustedName(prefix: string | undefined, name: string){ +function prefixAdjustedName(prefix: string | undefined, name: string) { return prefix ? `${prefix}_${name}` : name } diff --git a/core/utils.ts b/core/utils.ts index 8e1f3cc99..f8b68616c 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -306,7 +306,7 @@ export function resolveActionsConfigFilename(configFilename: string, configPath: } export function addDependenciesToActionDependencyTargets(action: actionsWithDependencies, resolvable: Resolvable) { - let dependencyTarget = resolvableAsTarget(resolvable); + const dependencyTarget = resolvableAsTarget(resolvable); if (!dependencyTarget.hasOwnProperty("includeDependentAssertions")) { // dependency `includeDependentAssertions` takes precedence over the config's `dependOnDependencyAssertions` dependencyTarget.includeDependentAssertions = action.dependOnDependencyAssertions; From ffb135ab15f008a322ecff6b0ab2a132fa9e3c89 Mon Sep 17 00:00:00 2001 From: Elias Kassell Date: Wed, 1 May 2024 11:39:40 +0100 Subject: [PATCH 10/12] Update cloudbuild file to default name, make yaml (#1722) --- cloudbuild-test.json | 13 ------------- cloudbuild.yaml | 8 ++++++++ 2 files changed, 8 insertions(+), 13 deletions(-) delete mode 100644 cloudbuild-test.json create mode 100644 cloudbuild.yaml diff --git a/cloudbuild-test.json b/cloudbuild-test.json deleted file mode 100644 index 54b95589e..000000000 --- a/cloudbuild-test.json +++ /dev/null @@ -1,13 +0,0 @@ -{ - "steps": [ - { - "name": "gcr.io/cloud-builders/bazel:5.4.0", - "entrypoint": "bash", - "args": ["./scripts/run_tests"] - } - ], - "timeout": "3600s", - "options": { - "machineType": "N1_HIGHCPU_8" - } -} diff --git a/cloudbuild.yaml b/cloudbuild.yaml new file mode 100644 index 000000000..b367973d0 --- /dev/null +++ b/cloudbuild.yaml @@ -0,0 +1,8 @@ +steps: + - name: gcr.io/cloud-builders/bazel:5.4.0 + entrypoint: bash + args: + - ./scripts/run_tests +options: + machineType: N1_HIGHCPU_8 +timeout: 3600s From 17a62f9ba765a9c429198785bab4c2a3fdcf16dc Mon Sep 17 00:00:00 2001 From: Elias Kassell Date: Tue, 7 May 2024 09:47:16 +0100 Subject: [PATCH 11/12] Make cloud build for dataform-open-source work for pull requests (#1728) * Update machine type and references to non dataform-open-source * Disable remote cache * Temporarily disable tests that require working BQ credentials * Make CLI test disablement only for run * Nit tidy --- .bazelrc | 11 +- cli/BUILD | 3 +- cli/index_test.ts | 126 +++++++++--------- cloudbuild.yaml | 2 +- scripts/create_secret | 2 +- scripts/decrypt_secret | 2 +- scripts/run_tests | 2 +- test_credentials/BUILD | 2 +- tests/integration/BUILD | 51 +++---- tests/integration/bigquery.spec.ts | 32 ++--- .../bigquery_project/dataform.json | 2 +- .../definitions/example_view.sqlx | 2 +- 12 files changed, 118 insertions(+), 119 deletions(-) diff --git a/.bazelrc b/.bazelrc index b6c3242af..73d6d3d95 100644 --- a/.bazelrc +++ b/.bazelrc @@ -3,10 +3,11 @@ build --test_output=errors --action_env="GTEST_COLOR=1" # TODO: Fix all the underlying issues here. build --incompatible_py3_is_default=false +# TODO(ekrekr): explore re-enabling these, if open source development is too slow. # The following flags enable the remote cache so action results can be shared # across machines, developers, and workspaces. -build:remote-cache --remote_cache=grpcs://remotebuildexecution.googleapis.com -build:remote-cache --remote_instance_name=projects/dataform-corp/instances/dataform-co -build:remote-cache --remote_timeout=3600 -build:remote-cache --auth_enabled=true -build:remote-cache --google_default_credentials=true +# build:remote-cache --remote_cache=grpcs://remotebuildexecution.googleapis.com +# build:remote-cache --remote_instance_name=projects/dataform-open-source/instances/dataform-co +# build:remote-cache --remote_timeout=3600 +# build:remote-cache --auth_enabled=true +# build:remote-cache --google_default_credentials=true diff --git a/cli/BUILD b/cli/BUILD index f0d5423e5..5353a8ff4 100644 --- a/cli/BUILD +++ b/cli/BUILD @@ -48,7 +48,8 @@ ts_test_suite( data = [ ":node_modules", "//packages/@dataform/core:package_tar", - "//test_credentials:bigquery.json", + # TODO(ekrekr): re-add these once we have working BQ credentials. + # "//test_credentials:bigquery.json", "@nodejs//:node", "@nodejs//:npm", ], diff --git a/cli/index_test.ts b/cli/index_test.ts index fd3ac80c3..a2f4a668b 100644 --- a/cli/index_test.ts +++ b/cli/index_test.ts @@ -154,13 +154,7 @@ defaultAssertionDataset: dataform_assertions // Initialize a project using the CLI, don't install packages. await getProcessResult( - execFile(nodePath, [ - cliEntryPointPath, - "init", - projectDir, - "dataform-integration-tests", - "US" - ]) + execFile(nodePath, [cliEntryPointPath, "init", projectDir, "dataform-open-source", "US"]) ); // Install packages manually to get around bazel read-only sandbox issues. @@ -219,14 +213,14 @@ select 1 as \${dataform.projectConfig.vars.testVar2} type: "table", enumType: "TABLE", target: { - database: "dataform-integration-tests", + database: "dataform-open-source", schema: "dataform_test_schema_suffix", name: "example" }, canonicalTarget: { schema: "dataform", name: "example", - database: "dataform-integration-tests" + database: "dataform-open-source" }, query: "\n\nselect 1 as testValue2\n", disabled: false, @@ -238,7 +232,7 @@ select 1 as \${dataform.projectConfig.vars.testVar2} warehouse: "bigquery", defaultSchema: "dataform", assertionSchema: "dataform_assertions", - defaultDatabase: "dataform-integration-tests", + defaultDatabase: "dataform-open-source", defaultLocation: "US", vars: { testVar1: "testValue1", @@ -250,69 +244,71 @@ select 1 as \${dataform.projectConfig.vars.testVar2} dataformCoreVersion: version, targets: [ { - database: "dataform-integration-tests", + database: "dataform-open-source", schema: "dataform", name: "example" } ] }); - // Dry run the project. - const runResult = await getProcessResult( - execFile(nodePath, [ - cliEntryPointPath, - "run", - projectDir, - "--credentials", - path.resolve(process.env.RUNFILES, "df/test_credentials/bigquery.json"), - "--dry-run", - "--json", - "--vars=testVar1=testValue1,testVar2=testValue2", - "--default-location=europe", - "--tags=someTag,someOtherTag" - ]) - ); + // TODO(ekrekr): re-enable this part of the test once we have working BQ credentials. - expect(runResult.exitCode).equals(0); + // // Dry run the project. + // const runResult = await getProcessResult( + // execFile(nodePath, [ + // cliEntryPointPath, + // "run", + // projectDir, + // "--credentials", + // path.resolve(process.env.RUNFILES, "df/test_credentials/bigquery.json"), + // "--dry-run", + // "--json", + // "--vars=testVar1=testValue1,testVar2=testValue2", + // "--default-location=europe", + // "--tags=someTag,someOtherTag" + // ]) + // ); - expect(JSON.parse(runResult.stdout)).deep.equals({ - actions: [ - { - fileName: "definitions/example.sqlx", - hermeticity: "HERMETIC", - tableType: "table", - target: { - database: "dataform-integration-tests", - name: "example", - schema: "dataform" - }, - tasks: [ - { - statement: - "create or replace table `dataform-integration-tests.dataform.example` as \n\nselect 1 as testValue2", - type: "statement" - } - ], - type: "table" - } - ], - projectConfig: { - assertionSchema: "dataform_assertions", - defaultDatabase: "dataform-integration-tests", - defaultLocation: "europe", - defaultSchema: "dataform", - warehouse: "bigquery", - vars: { - testVar1: "testValue1", - testVar2: "testValue2" - } - }, - runConfig: { - fullRefresh: false, - tags: ["someTag", "someOtherTag"] - }, - warehouseState: {} - }); + // expect(runResult.exitCode).equals(0); + + // expect(JSON.parse(runResult.stdout)).deep.equals({ + // actions: [ + // { + // fileName: "definitions/example.sqlx", + // hermeticity: "HERMETIC", + // tableType: "table", + // target: { + // database: "dataform-open-source", + // name: "example", + // schema: "dataform" + // }, + // tasks: [ + // { + // statement: + // "create or replace table `dataform-open-source.dataform.example` as \n\nselect 1 as testValue2", + // type: "statement" + // } + // ], + // type: "table" + // } + // ], + // projectConfig: { + // assertionSchema: "dataform_assertions", + // defaultDatabase: "dataform-open-source", + // defaultLocation: "europe", + // defaultSchema: "dataform", + // warehouse: "bigquery", + // vars: { + // testVar1: "testValue1", + // testVar2: "testValue2" + // } + // }, + // runConfig: { + // fullRefresh: false, + // tags: ["someTag", "someOtherTag"] + // }, + // warehouseState: {} + // }); }); }); diff --git a/cloudbuild.yaml b/cloudbuild.yaml index b367973d0..399db1b93 100644 --- a/cloudbuild.yaml +++ b/cloudbuild.yaml @@ -4,5 +4,5 @@ steps: args: - ./scripts/run_tests options: - machineType: N1_HIGHCPU_8 + machineType: E2_HIGHCPU_8 timeout: 3600s diff --git a/scripts/create_secret b/scripts/create_secret index 80371c694..e9d17ac07 100755 --- a/scripts/create_secret +++ b/scripts/create_secret @@ -3,7 +3,7 @@ gcloud kms encrypt \ --ciphertext-file=$1.enc \ --plaintext-file=$1 \ - --project=dataform-public \ + --project=dataform-open-source \ --keyring=dataform-builder-keyring \ --key=dataform-builder-key \ --location=global diff --git a/scripts/decrypt_secret b/scripts/decrypt_secret index 088572306..da7fe20b9 100755 --- a/scripts/decrypt_secret +++ b/scripts/decrypt_secret @@ -3,7 +3,7 @@ gcloud kms decrypt \ --ciphertext-file=$1.enc \ --plaintext-file=$1 \ - --project=dataform-public \ + --project=dataform-open-source \ --keyring=dataform-builder-keyring \ --key=dataform-builder-key \ --location=global diff --git a/scripts/run_tests b/scripts/run_tests index 960f16d65..f6e9093a9 100755 --- a/scripts/run_tests +++ b/scripts/run_tests @@ -6,4 +6,4 @@ bazel run @nodejs//:yarn bazel build @npm//tslint/bin:tslint && bazel-bin/external/npm/tslint/bin/tslint.sh --project . # Run all the tests -bazel test --config=remote-cache ... --build_tests_only --test_env=USE_CLOUD_BUILD_NETWORK=true +bazel test ... --build_tests_only --test_env=USE_CLOUD_BUILD_NETWORK=true diff --git a/test_credentials/BUILD b/test_credentials/BUILD index dfb1a18e2..c56fb73d6 100644 --- a/test_credentials/BUILD +++ b/test_credentials/BUILD @@ -6,7 +6,7 @@ gcloud_secret( name = "bigquery.json", testonly = 1, ciphertext_file = ":bigquery.json.enc", - project = "dataform-public", + project = "dataform-open-source", key = "dataform-builder-key", keyring = "dataform-builder-keyring", ) diff --git a/tests/integration/BUILD b/tests/integration/BUILD index 1d9230d80..d18713421 100644 --- a/tests/integration/BUILD +++ b/tests/integration/BUILD @@ -2,28 +2,29 @@ package(default_visibility = ["//visibility:public"]) load("//testing:index.bzl", "ts_test_suite") -ts_test_suite( - name = "tests", - srcs = [ - "utils.ts", - "bigquery.spec.ts", - ], - data = [ - "//test_credentials:bigquery.json", - "//tests/integration/bigquery_project:files", - "//tests/integration/bigquery_project:node_modules", - ], - deps = [ - "//cli/api", - "//cli/api/utils", - "//common/promises", - "//core", - "//protos:ts", - "//testing", - "//tests/utils", - "@npm//@types/chai", - "@npm//@types/long", - "@npm//@types/node", - "@npm//chai", - ], -) +# TODO(ekrekr): re-enable these tests once we have working BQ credentials. +# ts_test_suite( +# name = "tests", +# srcs = [ +# "utils.ts", +# "bigquery.spec.ts", +# ], +# data = [ +# "//test_credentials:bigquery.json", +# "//tests/integration/bigquery_project:files", +# "//tests/integration/bigquery_project:node_modules", +# ], +# deps = [ +# "//cli/api", +# "//cli/api/utils", +# "//common/promises", +# "//core", +# "//protos:ts", +# "//testing", +# "//tests/utils", +# "@npm//@types/chai", +# "@npm//@types/long", +# "@npm//@types/node", +# "@npm//chai", +# ], +# ) diff --git a/tests/integration/bigquery.spec.ts b/tests/integration/bigquery.spec.ts index 7cf29e2e7..6ef36a930 100644 --- a/tests/integration/bigquery.spec.ts +++ b/tests/integration/bigquery.spec.ts @@ -30,7 +30,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) // Drop schemas to make sure schema creation works. await dbadapter.execute( - "drop schema if exists `dataform-integration-tests.df_integration_test_eu_project_e2e` cascade" + "drop schema if exists `dataform-open-source.df_integration_test_eu_project_e2e` cascade" ); // Run the project. @@ -42,8 +42,8 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) // Check the status of action execution. const expectedFailedActions = [ - "dataform-integration-tests.df_integration_test_eu_assertions_project_e2e.example_assertion_fail", - "dataform-integration-tests.df_integration_test_eu_project_e2e.example_operation_partial_fail" + "dataform-open-source.df_integration_test_eu_assertions_project_e2e.example_assertion_fail", + "dataform-open-source.df_integration_test_eu_project_e2e.example_operation_partial_fail" ]; for (const actionName of Object.keys(actionMap)) { const expectedResult = expectedFailedActions.includes(actionName) @@ -57,13 +57,13 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) expect( actionMap[ - "dataform-integration-tests.df_integration_test_eu_assertions_project_e2e.example_assertion_fail" + "dataform-open-source.df_integration_test_eu_assertions_project_e2e.example_assertion_fail" ].tasks[1].errorMessage ).to.eql("bigquery error: Assertion failed: query returned 1 row(s)."); expect( actionMap[ - "dataform-integration-tests.df_integration_test_eu_project_e2e.example_operation_partial_fail" + "dataform-open-source.df_integration_test_eu_project_e2e.example_operation_partial_fail" ].tasks[0].errorMessage ).to.eql("bigquery error: Query error: Unrecognized name: invalid_column at [3:8]"); }); @@ -107,7 +107,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) const [incrementalRows, incrementalMergeRows] = await Promise.all([ getTableRows( { - database: "dataform-integration-tests", + database: "dataform-open-source", schema: "df_integration_test_eu_incremental_tables", name: "example_incremental" }, @@ -116,7 +116,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) ), getTableRows( { - database: "dataform-integration-tests", + database: "dataform-open-source", schema: "df_integration_test_eu_incremental_tables", name: "example_incremental_merge" }, @@ -153,7 +153,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) for (const expectedMetadata of [ { target: { - database: "dataform-integration-tests", + database: "dataform-open-source", schema: "df_integration_test_eu_dataset_metadata", name: "example_incremental" }, @@ -192,7 +192,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) }, { target: { - database: "dataform-integration-tests", + database: "dataform-open-source", schema: "df_integration_test_eu_dataset_metadata", name: "example_view" }, @@ -260,7 +260,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) await dfapi.run(dbadapter, executionGraph).result(); const view = keyBy(compiledGraph.tables, t => targetAsReadableString(t.target))[ - "dataform-integration-tests.df_integration_test_eu_evaluate.example_view" + "dataform-open-source.df_integration_test_eu_evaluate.example_view" ]; let evaluations = await dbadapter.evaluate(dataform.Table.create(view)); expect(evaluations.length).to.equal(1); @@ -269,7 +269,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) ); const materializedView = keyBy(compiledGraph.tables, t => targetAsReadableString(t.target))[ - "dataform-integration-tests.df_integration_test_eu_evaluate.example_materialized_view" + "dataform-open-source.df_integration_test_eu_evaluate.example_materialized_view" ]; evaluations = await dbadapter.evaluate(dataform.Table.create(materializedView)); expect(evaluations.length).to.equal(1); @@ -278,7 +278,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) ); const table = keyBy(compiledGraph.tables, t => targetAsReadableString(t.target))[ - "dataform-integration-tests.df_integration_test_eu_evaluate.example_table" + "dataform-open-source.df_integration_test_eu_evaluate.example_table" ]; evaluations = await dbadapter.evaluate(dataform.Table.create(table)); expect(evaluations.length).to.equal(1); @@ -287,7 +287,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) ); const operation = keyBy(compiledGraph.operations, t => targetAsReadableString(t.target))[ - "dataform-integration-tests.df_integration_test_eu_evaluate.example_operation" + "dataform-open-source.df_integration_test_eu_evaluate.example_operation" ]; evaluations = await dbadapter.evaluate(dataform.Operation.create(operation)); expect(evaluations.length).to.equal(1); @@ -296,7 +296,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) ); const assertion = keyBy(compiledGraph.assertions, t => targetAsReadableString(t.target))[ - "dataform-integration-tests.df_integration_test_eu_assertions_evaluate.example_assertion_pass" + "dataform-open-source.df_integration_test_eu_assertions_evaluate.example_assertion_pass" ]; evaluations = await dbadapter.evaluate(dataform.Assertion.create(assertion)); expect(evaluations.length).to.equal(1); @@ -305,7 +305,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) ); const incremental = keyBy(compiledGraph.tables, t => targetAsReadableString(t.target))[ - "dataform-integration-tests.df_integration_test_eu_evaluate.example_incremental" + "dataform-open-source.df_integration_test_eu_evaluate.example_incremental" ]; evaluations = await dbadapter.evaluate(dataform.Table.create(incremental)); expect(evaluations.length).to.equal(2); @@ -321,7 +321,7 @@ suite("@dataform/integration/bigquery", { parallel: true }, ({ before, after }) const target = (name: string) => ({ schema: "df_integration_test_eu", name, - database: "dataform-integration-tests" + database: "dataform-open-source" }); let evaluations = await dbadapter.evaluate( diff --git a/tests/integration/bigquery_project/dataform.json b/tests/integration/bigquery_project/dataform.json index 2dd461535..f0e88bf15 100644 --- a/tests/integration/bigquery_project/dataform.json +++ b/tests/integration/bigquery_project/dataform.json @@ -1,6 +1,6 @@ { "warehouse": "bigquery", - "defaultDatabase": "dataform-integration-tests", + "defaultDatabase": "dataform-open-source", "defaultSchema": "df_integration_test_eu", "assertionSchema": "df_integration_test_eu_assertions", "defaultLocation": "US", diff --git a/tests/integration/bigquery_project/definitions/example_view.sqlx b/tests/integration/bigquery_project/definitions/example_view.sqlx index 5aca37a58..3b2db2e33 100644 --- a/tests/integration/bigquery_project/definitions/example_view.sqlx +++ b/tests/integration/bigquery_project/definitions/example_view.sqlx @@ -4,7 +4,7 @@ config { columns: { val: { description: "val doc", - bigqueryPolicyTags: "projects/dataform-integration-tests/locations/eu/taxonomies/3546360117715097846/policyTags/6995291324014833226" + bigqueryPolicyTags: "projects/dataform-open-source/locations/eu/taxonomies/3546360117715097846/policyTags/6995291324014833226" } }, bigquery: { From a9910b8736f6ed9e0d816098bc97ce9990546b37 Mon Sep 17 00:00:00 2001 From: Muhammad Tauseeq Date: Tue, 7 May 2024 09:40:04 +0000 Subject: [PATCH 12/12] fix shadow beforeEach call --- core/main_test.ts | 78 ++++++++++++----------------------------------- 1 file changed, 19 insertions(+), 59 deletions(-) diff --git a/core/main_test.ts b/core/main_test.ts index 553e7bdf0..3d11c97b2 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -1287,11 +1287,19 @@ config { type: "assertion", } select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + }); test("When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies", () => { fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), -`config { + ` +config { type: "table", dependOnDependencyAssertions: true, dependencies: ["A"] @@ -1477,35 +1485,12 @@ SELECT 1 as test expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" properties are not allowed. Dependency A has different values set for this property.`); }) - suite("Action configs", ({ beforeEach }) => { - beforeEach("Create temporary dir and files", () => { - projectDir = tmpDirFixture.createNewTmpDir(); - fs.writeFileSync( - path.join(projectDir, "workflow_settings.yaml"), - dumpYaml(dataform.WorkflowSettings.create(testConfig)) - ); - fs.mkdirSync(path.join(projectDir, "definitions")) - fs.writeFileSync(path.join(projectDir, "definitions/A.sql"), "SELECT 1 as test"); - fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sql"), "SELECT test from A"); - fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); - fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); - fs.writeFileSync( - path.join(projectDir, `definitions/notebook.ipynb`), - EMPTY_NOTEBOOK_CONTENTS - ); - }); - + suite("Action configs", () => { test(`When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies`, () => { fs.writeFileSync( path.join(projectDir, "definitions/actions.yaml"), ` actions: -- table: - filename: A.sql -- assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - view: filename: B.sql dependOnDependencyAssertions: true @@ -1525,10 +1510,9 @@ actions: ); const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - - expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1537,12 +1521,6 @@ actions: path.join(projectDir, "definitions/actions.yaml"), ` actions: -- table: - filename: A.sql -- assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - view: filename: B.sql dependencyTargets: @@ -1563,9 +1541,9 @@ actions: const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(2); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1574,12 +1552,6 @@ actions: path.join(projectDir, "definitions/actions.yaml"), ` actions: -- table: - filename: A.sql -- assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - view: filename: B.sql dependOnDependencyAssertions: true @@ -1620,12 +1592,6 @@ actions: path.join(projectDir, "definitions/actions.yaml"), ` actions: -- table: - filename: A.sql -- assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - view: filename: B.sql dependOnDependencyAssertions: false @@ -1656,9 +1622,9 @@ actions: const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); - expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(3); - expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(2); - expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(4); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(4); expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); }); @@ -1667,12 +1633,6 @@ actions: path.join(projectDir, "definitions/actions.yaml"), ` actions: -- table: - filename: A.sql -- assertion: - filename: A_assert.sql - dependencyTargets: - - name: A - view: filename: B.sql dependOnDependencyAssertions: true