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/test mutually exclusive ranges #166

Merged
merged 7 commits into from
Dec 2, 2019

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Oct 9, 2019

Test failing because of this issue. Will wait for that fix rather than working around the issue.

README.md Outdated Show resolved Hide resolved
@clrcrl clrcrl requested a review from jthandy October 9, 2019 18:41
@jthandy
Copy link
Member

jthandy commented Oct 10, 2019

@drewbanin I'm not going to be able to look at this for like another week. Any chance you're able to help out here? Sorry, I know I'm useless right now.

@drewbanin drewbanin requested review from drewbanin and removed request for jthandy October 10, 2019 01:47
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

This is groovy! Couple of small comments, but largely LGTM.

I think the logic in the where clause could benefit from additional attention to better handle cases when lower_bound_column or upper_bound_column are errantly null. Keen to discuss!

Also: That CI issue should be resolved - the tests should run on your next push

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
macros/schema_tests/test_mutually_exclusive_ranges.sql Outdated Show resolved Hide resolved
),

validation_errors as (
-- we want to return records where our assumptions are NOT true, so we'll use
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? It seems like you did the opposite of this in the code below! I think it would make sense to put one big not around the entire predicate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrote this to make it cleaner

macros/schema_tests/test_mutually_exclusive_ranges.sql Outdated Show resolved Hide resolved
@clrcrl
Copy link
Contributor Author

clrcrl commented Oct 15, 2019

A few other things:

  1. Is mutually_exclusive the right term here? Is it known enough? Should we call it non_overlapping_ranges instead?
  2. I thought about getting extremely persnickety and implementing three operators, i.e.:
gaps:not_allowed --> lower_bound  = previous_upper_bound
gaps:allowed     --> lower_bound <= previous_upper_bound
gaps:required    --> lower_bound <  previous_upper_bound

example of gaps:not_allowed:

lower_bound upper_bound
0 1
1 2
2 3

example of gaps:allowed

lower_bound upper_bound
0 1
2 3
3 4

example of gaps:required

lower_bound upper_bound
0 1
2 3
4 5

There's probably a better way to express that, not sure if it's worth implementing. I can imagine ranges (especially timestamp ones) where you actually want things to not overlap so you don't have a fan out in any downstream joins, but 🤷‍♀

  1. Should there be an extra optional arg for order_by? I just assumed you could order by the lower_bound, which feels pretty safe for now.
    Never mind -- making the lower_bound not null means that this is a reasonable assumption!

@clrcrl clrcrl force-pushed the feature/test-mutually-exclusive-ranges branch from f827cb3 to ee0c4d7 Compare October 16, 2019 14:26
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

This looks excellent - ship it!

@clrcrl clrcrl force-pushed the feature/test-mutually-exclusive-ranges branch from b2ee6cf to ee0c4d7 Compare December 2, 2019 16:51
@clrcrl clrcrl merged commit 860d6d2 into master Dec 2, 2019
@clrcrl clrcrl deleted the feature/test-mutually-exclusive-ranges branch December 2, 2019 17:39
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.

None yet

3 participants