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

Fix uniqueKey assertions for views, incremental tables #1836

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

bmagyarkuti
Copy link
Contributor

@bmagyarkuti bmagyarkuti commented Sep 10, 2024

The Bug

This PR fixes a bug where uniqueKey assertions on views and incremental tables caused dataform run to exit with a query error. uniqueKey assertions on tables, as well as all uniqueKeys assertions still worked. Affected versions: 3.0.1, 3.0.2. I don't know whether executions on GCP were also affected.

Steps to reproduce

  1. Create an empty dataform project using version 3.0.1 or 3.0.2 of @dataform/core.
  2. Add the following definition:
config {
  type: "view",
  name: "my table",
  assertions: {
    uniqueKey: ["id"]
  }
}

SELECT 1 as id
  1. Execute dataform run. Observe the following error message:
Table created:  dataform.my table [view]
Assertion failed:  dataform_assertions.dataform_my table_assertions_uniqueKey_0
  >
  >       create or replace view `mbarna-mongo-to-bq-dataflow.dataform_assertions.dataform_my table_assertions_uniqueKey_0` as
  > SELECT
  >   *
  > FROM (
  >   SELECT
  >     TableAssertionsConfig,
  >     COUNT(1) AS index_row_count
  >   FROM `mbarna-mongo-to-bq-dataflow.dataform.my table`
  >   GROUP BY TableAssertionsConfig
  >   ) AS data
  > WHERE index_row_count > 1
  >
  bigquery error: Unrecognized name: TableAssertionsConfig at [7:5]

The Fix

I don't fully understand what happened. The query complains about an undefined column TableAssertionsConfig, and the newly refactored implementations for view and incremental_table contain this hardcoded string for uniqueKey assertions. I'm not sure what the hardcoded string is meant to be referring to. Just replacing the hardcoded string with the user-supplied column names fixes the issue.

The Tests

The suite in core/main_test.ts tested assertions for views, tables and incremental_tables, for the following assertions: uniqueKeys, nonNull, rowConditions. uniqueKey assertions were untested, I assume because they're incompatible with a uniqueKeys assertion. I extended the test suite to also include tests for uniqueKey assertions, for all three tested table kinds. I worked around the incompatibility by introducing a second sample file. Apart from the different assertions block, the second file's contents are identical to the contents of the first file.

This has confirmed the bug, as well as the fix.

I nominate @Ekrekr as reviewer.

@lewish lewish requested a review from Ekrekr October 7, 2024 09:12
@Ekrekr
Copy link
Contributor

Ekrekr commented Oct 7, 2024

Thank you so much for this fix! It would have been approved earlier, but there's been a lot on.

It looks good - I'll merge main in with this and make sure the remote tests pass before merging.

Fixes #1835
Fixes #1804
Fixes b/371514877

@Ekrekr Ekrekr merged commit 5f1b9a0 into dataform-co:main Oct 7, 2024
1 of 2 checks passed
benjaminwestern pushed a commit to benjaminwestern/dataform-testing that referenced this pull request Nov 9, 2024
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.

2 participants