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

[Bug] unit tests' comparisons are sometimes sensitive to the order of records returned #10167

Closed
2 tasks done
joellabes opened this issue May 17, 2024 · 5 comments · Fixed by #10202
Closed
2 tasks done
Assignees
Labels
backport 1.8.latest bug Something isn't working Medium Severity bug with minor impact that does not have resolution timeframe requirement unit tests Issues related to built-in dbt unit testing functionality

Comments

@joellabes
Copy link
Contributor

joellabes commented May 17, 2024

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Normally, dbt's unit tests don't care whether rows are ordered the same as the expect block or not.

id 
---
1
2
3

is just as good as

id
---
3
2
1

as far as

...
expect: 
  rows:
    - {"id": 2}
    - {"id": 3}
    - {"id": 1}

is concerned.

I have a use case which is order dependent, but I don't know why.

Expected Behavior

Order to not matter

Steps To Reproduce

Unit test yaml:

unit_tests:
  - name: reworked_compare_identical_tables_multiple_null_pk_with_duplicate_rows
    description: All rows with a null ID are identical. They should be returned as individual rows instead of being combined
    model: unit_reworked_compare
    
    given:
      - input: ref('unit_test_model_a')
        rows:
          - { "id": , "col1": "abc", "col2": "def" }
          - { "id": , "col1": "abc", "col2": "def" }
          - { "id": 3, "col1": "nop", "col2": "qrs" }
      - input: ref('unit_test_model_b')
        rows:
          - { "id": , "col1": "abc", "col2": "def" }
          - { "id": , "col1": "abc", "col2": "def" }
          - { "id": , "col1": "abc", "col2": "def" }
          - { "id": 3, "col1": "nop", "col2": "qrs" }
        
    expect:
      rows:
        - {"dbt_audit_row_status": 'nonunique_pk', 'id': , dbt_audit_num_rows_in_status: 3}
        - {"dbt_audit_row_status": 'nonunique_pk', 'id': , dbt_audit_num_rows_in_status: 3}
        - {"dbt_audit_row_status": 'nonunique_pk', 'id': , dbt_audit_num_rows_in_status: 3}
        - {"dbt_audit_row_status": 'identical', 'id': 3, dbt_audit_num_rows_in_status: 1}

    overrides:
      vars:
        reworked_compare__columns: ['id', 'col1', 'col2']
        reworked_compare__event_time:
        reworked_compare__primary_key_columns: ['id']

The tested model (which is just a call to a macro)

--models/unit_reworked_compare.sql
{{ 
    audit_helper.reworked_compare(
        "select * from " ~ ref('unit_test_model_a'),
        "select * from " ~ ref('unit_test_model_b'),
        primary_key_columns=var('reworked_compare__primary_key_columns'),
        columns=var('reworked_compare__columns'),
        event_time=var('reworked_compare__event_time')
    ) 
}}
--models/unit_test_model_a
select 12 as id, 22 as id_2, 'xyz' as col1, 'tuv' as col2, 123 as col3, {{ dbt.current_timestamp() }} as created_at
--models/unit_test_model_b
select 12 as id, 22 as id_2, 'xyz' as col1, 'tuv' as col2, 123 as col3, {{ dbt.current_timestamp() }} as created_at

(Yes, unit_test_model_a and unit_test_model_b are identical! They're not used in production contexts, just to define the columns so they can be overridden by the actual test)

Log output when I run `dbt test -s :

23:23:49  Failure in unit_test reworked_compare_identical_tables_multiple_null_pk_with_duplicate_rows (models/unit_test_wrappers/unit_reworked_compare.yml)
23:23:49    

actual differs from expected:

@@ ,DBT_AUDIT_ROW_STATUS,ID  ,DBT_AUDIT_NUM_ROWS_IN_STATUS
---,nonunique_pk        ,null,3
---,nonunique_pk        ,null,3
---,nonunique_pk        ,null,3
   ,identical           ,3   ,1
+++,nonunique_pk        ,null,3
+++,nonunique_pk        ,null,3
+++,nonunique_pk        ,null,3

But if I reorder the expect block so that the identical row is on top:

    expect:
      rows:
        - {"dbt_audit_row_status": 'identical', 'id': 3, dbt_audit_num_rows_in_status: 1}
        - {"dbt_audit_row_status": 'nonunique_pk', 'id': , dbt_audit_num_rows_in_status: 3}
        - {"dbt_audit_row_status": 'nonunique_pk', 'id': , dbt_audit_num_rows_in_status: 3}
        - {"dbt_audit_row_status": 'nonunique_pk', 'id': , dbt_audit_num_rows_in_status: 3}

The test passes.

Compiled code:
reworked_compare_identical_tables_multiple_null_pk_with_duplicate_rows.txt

Relevant log output

No response

Environment

00:08:41  dbt version: 1.8.0-b3
00:08:41  python version: 3.11.0
00:08:41  python path: /Users/joel/venvs/dbt-dev/bin/python3
00:08:41  os info: macOS-14.4.1-x86_64-i386-64bit

Which database adapter are you using with dbt?

Snowflake

Additional Context

We believe this is happening when there are multiple explicit null values in the expected output.

Even though the tested columns are all identical, the extra columns that the results return do vary. I made a Loom of a different unit test which is easier to demonstrate with: https://www.loom.com/share/23b636f08019437eb2b09c912e9723fb

You can see all of my assorted tests here.

Blocking audit_helper PR dbt-labs/dbt-audit-helper#99

@joellabes joellabes added bug Something isn't working triage labels May 17, 2024
@jtcohen6 jtcohen6 added the unit tests Issues related to built-in dbt unit testing functionality label May 17, 2024
@dbeatty10
Copy link
Contributor

Not sure if it is related or not, but in dbt-labs/dbt-redshift#821, the rows with non-null values also needed to come prior to the rows with nulls.

@graciegoheen
Copy link
Contributor

@joellabes Which database are you seeing this error in?

@dbeatty10
Copy link
Contributor

Here's some minimal reproducible examples using dbt-postgres:

Reprex

models/my_input_model.sql

select 1 as id, 'some string' as status

models/my_model.sql

select * from {{ ref("my_input_model") }}

models/unit_tests.yml

unit_tests:
  #
  - name: unexpected_test_1
    model: my_model
    given:
      - input: ref("my_input_model")
        rows:
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}
          - {"id": 3, "status": 'A'}
    expect:
        rows:
          - {"id": 3, "status": 'A'}
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}

  #
  - name: unexpected_test_2
    model: my_model
    given:
      - input: ref("my_input_model")
        rows:
          - {"id": 3, "status": 'A'}
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}
    expect:
        rows:
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}
          - {"id": 3, "status": 'A'}

  #
  - name: expected_test_0
    model: my_model
    given:
      - input: ref("my_input_model")
        rows:
          - {"id": 3, "status": 'A'}
          - {"id":  , "status": 'B'}
    expect:
        rows:
          - {"id":  , "status": 'B'}
          - {"id": 3, "status": 'A'}

  #
  - name: expected_test_1
    model: my_model
    given:
      - input: ref("my_input_model")
        rows:
          - {"id": 3, "status": 'A'}
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}
    expect:
        rows:
          - {"id": 3, "status": 'A'}
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}

  #
  - name: expected_test_2
    model: my_model
    given:
      - input: ref("my_input_model")
        rows:
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}
          - {"id": 3, "status": 'A'}
    expect:
        rows:
          - {"id":  , "status": 'B'}
          - {"id":  , "status": 'B'}
          - {"id": 3, "status": 'A'}

Run this command:

dbt build -s +my_model

@dbeatty10 dbeatty10 removed the triage label May 17, 2024
@joellabes
Copy link
Contributor Author

Amusingly, I also believe that each adapter has different order needs! This test fails on Redshift but passes locally on Snowflake: https://app.circleci.com/pipelines/github/dbt-labs/dbt-audit-helper/416/workflows/64b0d0f6-8cd8-48cf-bac2-e462987d69fa/jobs/409 (and presumably vice versa, but I haven't 100% tested that)

@graciegoheen graciegoheen added Medium Severity bug with minor impact that does not have resolution timeframe requirement backport 1.8.latest labels May 20, 2024
@graciegoheen
Copy link
Contributor

graciegoheen commented May 20, 2024

Notes from estimation:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.8.latest bug Something isn't working Medium Severity bug with minor impact that does not have resolution timeframe requirement unit tests Issues related to built-in dbt unit testing functionality
Projects
None yet
5 participants