Fix at_least_one test when group_by_columns is configured

[katieclaiborne-duet]

resolves #838

Problem

The pruning logic introduced in #776 disrupted the grouping behavior in the at_least_one test.

Specifically, the initial pruned_rows CTE includes a limit 1 statement, which selects a single record where the target column is not null.

As a result, the test passes inappropriately when the group_by_columns parameter is configured, as long as the target column has at least one not-null value.

Essentially, the group_by_columns parameter currently has no effect on the test. It evaluates target column values as a single group, regardless of whether or not the parameter is present.

Solution

Restore the at_least_one test to its prior state.

Since the goal of #776 was to improve the performance of the test, I also considered a set-logic approach to evaluating a column at the group level:

• identify all group-by column values
• identify group-by column values with at least one not-null target column value
• return group-by column values without at least one not-null target column value as errors

In BigQuery, however, this version of the test took twice as long as the simple reverted version.

Checklist

• This code is associated with an issue which has been triaged and accepted for development.
• I have read the contributing guide and understand what's expected of me
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have updated the README.md (if applicable)

[katieclaiborne-duet]

@dbeatty10, would you mind to review when you have a chance?

[dbeatty10]

A crucial step before merging this would be the addition of test(s) as described in #922 (comment)

[dbeatty10]

Thanks for raising this PR @katieclaiborne-duet 🤩

I used #934 to quickly check that the new test you added correctly surfaces the error case discovered in #838. Looks good ✅

It would be nice to keep the optimization and also fix the bug 🤞

When group_by_columns is an empty list, is there a way to keep the optimizations in #776 but avoid the bug reported in #838?

[dbeatty10]

Background

The at_least_one data test was originally added in #32. Then #633 / 78afedd added the group_by_columns argument, but no test was added that would catch a future regression. Then #776 accidentally added a regression, and this was reported in #838.

[katieclaiborne-duet]

When group_by_columns is an empty list, is there a way to keep the optimizations in #776 but avoid the bug reported in #838?

Good idea! Let me know what you think of this latest pass.

[dbeatty10]

This looks awesome @katieclaiborne-duet 🤩

Thank you for creating a relevant test for this bug and implementing a targeted fix 🧠

[tffragoso]

Thank you so much for the fix, @katieclaiborne-duet! 🙌