dbt-labs / dbt-labs/dbt-utils

`mutually_exclusive_ranges` Test Fails for Duplicated Zero-Length Ranges

Open
#981 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug triage
Dominant language
Makefile
Stars
1.8k
Forks
632
PR merge metrics
No merged PRs in 30d

Description

Describe the bug

The mutually_exclusive_ranges test currently flags duplicated zero-length ranges as failing rows, even when zero_length_range_allowed=True, which might be unexpected and/or undesired.

As a simple illustration of this, consider the following table:

trans_id start_date end_date
0 2000-01-01 2000-01-01
1 2000-01-01 2000-01-01

Mathematically speaking, these ranges are not overlapping, since both are zero-length. However, with the current implementation of mutually_exclusive_ranges, at least one of these rows is marked as a failure, which may be unexpected (it was for me at least :) ).

Although there are absolutely valid use cases for flagging duplicated zero-length rows like the above, this behavior should probably be toggleable.

Steps to reproduce

Here’s a stand-alone Python 3.10 script demonstrating this behavior, using jinja2==3.1.5 pandas==2.2.3 duckdb==1.1.3:

from typing import Callable

import duckdb
import pandas as pd
from jinja2 import Environment


def main():
    # Copied from https://github.com/dbt-labs/dbt-utils/blob/5c9dc0d43265cb86c5f69954e6d739dc7a1974c3/macros/generic_tests/mutually_exclusive_ranges.sql#L5:
    test_template_str = """
    {% macro mutually_exclusive_ranges(model, lower_bound_column, upper_bound_column, partition_by=None, gaps='allowed', zero_length_range_allowed=False) %}
    {% if gaps == 'not_allowed' %}
        {% set allow_gaps_operator='=' %}
        {% set allow_gaps_operator_in_words='equal_to' %}
    {% elif gaps == 'allowed' %}
        {% set allow_gaps_operator='<=' %}
        {% set allow_gaps_operator_in_words='less_than_or_equal_to' %}
    {% elif gaps == 'required' %}
        {% set allow_gaps_operator='<' %}
        {% set allow_gaps_operator_in_words='less_than' %}
    {% else %}
        {{ exceptions.raise_compiler_error(
            "`gaps` argument for mutually_exclusive_ranges test must be one of ['not_allowed', 'allowed', 'required'] Got: '" ~ gaps ~"'.'"
        ) }}
    {% endif %}
    {% if not zero_length_range_allowed %}
        {% set allow_zero_length_operator='<' %}
        {% set allow_zero_length_operator_in_words='less_than' %}
    {% elif zero_length_range_allowed %}
        {% set allow_zero_length_operator='<=' %}
        {% set allow_zero_length_operator_in_words='less_than_or_equal_to' %}
    {% else %}
        {{ exceptions.raise_compiler_error(
            "`zero_length_range_allowed` argument for mutually_exclusive_ranges test must be one of [true, false] Got: '" ~ zero_length_range_allowed ~"'.'"
        ) }}
    {% endif %}

    {% set partition_clause="partition by " ~ partition_by if partition_by else '' %}

    with window_functions as (

        select
            {% if partition_by %}
            {{ partition_by }} as partition_by_col,
            {% endif %}
            {{ lower_bound_column }} as lower_bound,
            {{ upper_bound_column }} as upper_bound,

            lead({{ lower_bound_column }}) over (
                {{ partition_clause }}
                order by {{ lower_bound_column }}, {{ upper_bound_column }}
            ) as next_lower_bound,

            lead(1) over (
                {{ partition_clause }}
                order by {{ lower_bound_column }}, {{ upper_bound_column }}
            ) is null as is_last_record_alternative,

            row_number() over (
                {{ partition_clause }}
                order by {{ lower_bound_column }} desc, {{ upper_bound_column }} desc
            ) = 1 as is_last_record

        from {{ model }}

    ),

    calc as (
        -- We want to return records where one of our assumptions fails, so we'll use
        -- the `not` function with `and` statements so we can write our assumptions more cleanly
        select
            *,

            -- For each record: lower_bound should be < upper_bound.
            -- Coalesce it to return an error on the null case (implicit assumption
            -- these columns are not_null)
            coalesce(
                lower_bound {{ allow_zero_length_operator }} upper_bound,
                false
            ) as lower_bound_{{ allow_zero_length_operator_in_words }}_upper_bound,

            -- For each record: upper_bound {{ allow_gaps_operator }} the next lower_bound.
            -- Coalesce it to handle null cases for the last record.
            coalesce(
                upper_bound {{ allow_gaps_operator }} next_lower_bound,
                is_last_record,
                false
            ) as upper_bound_{{ allow_gaps_operator_in_words }}_next_lower_bound

        from window_functions

    ),

    validation_errors as (

        select
            *
        from calc

        where not(
            -- THE FOLLOWING SHOULD BE TRUE --
            lower_bound_{{ allow_zero_length_operator_in_words }}_upper_bound
            and upper_bound_{{ allow_gaps_operator_in_words }}_next_lower_bound
        )
    )

    select * from validation_errors
    {% endmacro %}
    """

    # Test data with two zero-length ranges occurring on the same day:
    df = pd.DataFrame(
        {
            "trans_id": [0, 1],
            "start_date": 2 * [pd.to_datetime("2000-01-01").date()],
            "end_date": 2 * [pd.to_datetime("2000-01-01").date()],
        }
    )

    # Render test query for `df` defined above, allowing for zero-length ranges:
    test_renderer: Callable = (
        Environment().from_string(test_template_str).module.mutually_exclusive_ranges
    )
    test_query = test_renderer(
        model="df",
        lower_bound_column="start_date",
        upper_bound_column="end_date",
        zero_length_range_allowed=True,
    )

    print(duckdb.sql(test_query))

    return None


if __name__ == "__main__":
    main()
Expected results
lower_bound upper_bound next_lower_bound is_last_record lower_bound_less_than_or_equal_to_upper_bound upper_bound_less_than_or_equal_to_next_lower_bound

i.e. an empty relation.

Actual results
lower_bound upper_bound next_lower_bound is_last_record lower_bound_less_than_or_equal_to_upper_bound upper_bound_less_than_or_equal_to_next_lower_bound
0 2000-01-01 2000-01-01 NaT

i.e. the duplicated zero length range 2000-01-01 to 2000-01-01 is considered to overlap with itself.

System information

The contents of your packages.yml file:

N/A, since provided snippet directly renders mutually_exclusive_ranges test with jinja2, without using dbt.

Which database are you using dbt with?

  • postgres
  • [x ] redshift
  • bigquery
  • snowflake
  • [x ] other (specify: duckdb)

The output of dbt --version:

N/A, since provided snippet directly renders mutually_exclusive_ranges test with jinja2, without using dbt.

Additional context

I believe the root cause of this issue lies in how is_last_record is computed in mutually_exclusive_ranges:

row_number() over (
    {{ partition_clause }}
    order by {{ lower_bound_column }} desc, {{ upper_bound_column }} desc
) = 1 as is_last_record

This assumes that ordering by {{ lower_bound_column }} desc, {{ upper_bound_column }} desc always produces the exact reverse order of {{ lower_bound_column }}, {{ upper_bound_column }}. However, this isn't true for duplicated zero-length ranges, where the asc and desc ordering of records are identical. The net result of this is that some records are assigned a null next_lower_bound, but with a false is_last_record flag, causing these rows to be labelled as 'failing'.

To address this edge case, I suggest calculating is_last_record by ordering records in the same way as used to compute next_lower_bound, like so:

-- If no following record, `lead(1)` is null:
lead(1) over (
    {{ partition_clause }}
    -- No longer `desc` sorting:
    order by {{ lower_bound_column }}, {{ upper_bound_column }}
) is null as is_last_record,

A new argument should be introduced to allow users to toggle between this updated behavior and the current behavior.

Are you interested in contributing the fix?

Yes, I am interested in contributing this fix. I'll open a PR re: this issue later today if I find the time.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with macros/generic_tests/mutually_exclusive_ranges.sql, especially the is_last_record and next_lower_bound window calculations described in the issue. Verify the behavior with duplicated zero-length ranges when zero_length_range_allowed is true; done means those rows no longer fail, while the requested toggle preserves the existing behavior when selected.

Written by the indexing model from the issue text.

Assessment

Tech stack
sql
Domain
databases, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.