Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: dependent assertions as dependencies #1720

Merged
merged 12 commits into from
May 7, 2024

Conversation

Tuseeq1
Copy link
Member

@Tuseeq1 Tuseeq1 commented Apr 19, 2024

Feature: Dependent assertions as dependencies.

Summary:

This PR introduces changes that will allow users to add dependent assertions as dependencies.
This required a new ActionAssertionMap that holds information regarding assertions that depend on each action. This map is updated while create assertions.
New flags for Table and Operation to allow managing dependent assertions. These flags are updated for each dependency when its added in the Action either through config or ref(both happens in same place).
Lastly, when dependencies are qualified for compiledGraph, we add dependent assertions as dependencies using the ActionAssertionMap and the flags.

This PR also introduces new tests to test this feature and minor updates to some old test to adjust to new flags being introduces.

#core #feature #dependentAssertionsAsDependencies

@Tuseeq1 Tuseeq1 requested review from BenBirt and Ekrekr April 19, 2024 17:01
@Tuseeq1 Tuseeq1 self-assigned this Apr 19, 2024
Copy link
Contributor

@Ekrekr Ekrekr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a few outstanding comments in the design doc, and we haven't done a review session to close them, which we should do before this is merged!

Why is it just operations and tables that can have dependOnDependencyAssertions? Shouldn't all action types (excluding declarations) be able to require directly downstream assertions to have completed in order to run?

core/utils.ts Outdated Show resolved Hide resolved
core/common.ts Outdated Show resolved Hide resolved
core/actions/operation.ts Outdated Show resolved Hide resolved
core/actions/table.ts Outdated Show resolved Hide resolved
core/actions/table.ts Outdated Show resolved Hide resolved
core/actions/operation.ts Outdated Show resolved Hide resolved
@Tuseeq1
Copy link
Member Author

Tuseeq1 commented Apr 22, 2024

There's quite a few outstanding comments in the design doc, and we haven't done a review session to close them, which we should do before this is merged!

Why is it just operations and tables that can have dependOnDependencyAssertions? Shouldn't all action types (excluding declarations) be able to require directly downstream assertions to have completed in order to run?

We have 5 types of actions Assertions, Declaration, Notebook, Operations, and Tables.

  1. Assertions:
    We decided not to have this option for assertions to avoid circler graphs and avoid complexion as mentioned in DD.
  2. Declaration dont have dependencies
  3. Notebooks can only use action.yaml to set config and as we discussed in chat that currently we dont have this option.

that leave only Tables and Operations.

@Tuseeq1 Tuseeq1 requested a review from Ekrekr April 22, 2024 15:00
@Tuseeq1 Tuseeq1 requested a review from kolina April 25, 2024 17:49
Copy link
Contributor

@Ekrekr Ekrekr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is pretty massive! If you want to speed up the review process, I'd suggest splitting it up a bit - up to you. For example:

  1. add to the proto definition and ITarget
  2. add support the option in the SQLX config
  3. support the option in ref

core/actions/assertion.ts Show resolved Hide resolved
core/actions/notebook.ts Outdated Show resolved Hide resolved
core/common.ts Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/session.ts Outdated Show resolved Hide resolved
@@ -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./
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: this is kind of gross, we should fix it by not using JSON stringify like this https://github.com/dataform-co/dataform/blob/754d582f4f588b3c812c40a38ec839607ec3d4f0/core/utils.ts#L129C42-L129C53

core/common.ts Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/session.ts Outdated Show resolved Hide resolved
core/session.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
protos/configs.proto Outdated Show resolved Hide resolved
@Tuseeq1 Tuseeq1 requested a review from Ekrekr April 29, 2024 14:42
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
core/session.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
@Tuseeq1 Tuseeq1 requested a review from kolina April 30, 2024 13:27
core/actions/index.ts Outdated Show resolved Hide resolved
core/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Ekrekr Ekrekr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version should be increased here, for publishing the changes https://github.com/dataform-co/dataform/blob/main/version.bzl

core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
core/main_test.ts Outdated Show resolved Hide resolved
@Tuseeq1 Tuseeq1 requested a review from Ekrekr May 2, 2024 14:23
@Ekrekr
Copy link
Contributor

Ekrekr commented May 3, 2024

I've got most of the cloud build tests working in https://github.com/dataform-co/dataform, I think next time you push they should pop up!

If not, we can just run ./scripts/run_tests locally before merging.

Tuseeq1 and others added 3 commits May 6, 2024 12:24
* 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
Copy link
Contributor

@Ekrekr Ekrekr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something weird is up with the PR - I shouldn't be seeing the diff from the new code in main. Something to do with the way the merge was done
image

But it doesn't matter too much anyway, because it will be squashed on merge.

@Tuseeq1 Tuseeq1 merged commit e3237b9 into main May 7, 2024
4 checks passed
@Tuseeq1 Tuseeq1 deleted the dependentAssertionDependencies branch May 7, 2024 10:15
moker-spaghetti pushed a commit to moker-spaghetti/dataform that referenced this pull request May 26, 2024
* Feature: dependent assertions as dependencies

* Action.yaml support for dependent assertions

* Review updates

* spell check and ActionMap

* dependency duplication check O(n)

* Formating

* includeAssertionsForDependencyMap changed to includeAssertionsForDependency

* review fixes

* Lint fixes

* fix shadow beforeEach call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants