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

Add sequential values test #318

Closed
wants to merge 5 commits into from
Closed

Add sequential values test #318

wants to merge 5 commits into from

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Jan 4, 2021

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is master
  • new functionality — please ensure the base branch is the latest dev/ branch
  • (possibly) a breaking change — please ensure the base branch is the latest dev/ branch

Description & motivation

This macro builds on work done in #264, to add a general test that a column has sequential values (i.e. an arithmetic series)

It's a little different to the exact functionality of that test, but I think this one is more generally useful

🚨 Note: when getting the tests passing, I noticed that the date_add macro has some strange behavior on BigQuery — we use datetime_add, and cast to a datetime rather than a timestamp (thus losing any timezone info). I thought it would be preferential to use timestamp_add here instead, which meant I ended up touching some other files. @jtcohen6 — I'd really like your eyes on that part of the code

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@clrcrl clrcrl requested a review from jtcohen6 January 4, 2021 18:50
@clrcrl clrcrl mentioned this pull request Jan 4, 2021
11 tasks
@clrcrl clrcrl marked this pull request as ready for review January 4, 2021 19:04
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

I like it!

macros/schema_tests/sequential_values.sql Show resolved Hide resolved
macros/cross_db_utils/dateadd.sql Show resolved Hide resolved
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

LGTM!

@clrcrl clrcrl changed the title Sequential values Add sequential values test Jan 11, 2021
@clrcrl clrcrl force-pushed the dev/0.7.0 branch 4 times, most recently from bbba960 to 60a3b3c Compare January 11, 2021 15:52
@clrcrl
Copy link
Contributor Author

clrcrl commented Jan 11, 2021

huh, this works locally, and worked before I cleaned up this branch 🤔

@clrcrl
Copy link
Contributor Author

clrcrl commented Jan 11, 2021

:oof:

Database Error in model test_dateadd (models/cross_db_utils/test_dateadd.sql)
  TIMESTAMP_ADD does not support the MONTH date part at [36:9]

OK leaving this for now, it's chewing up too much time

@clrcrl
Copy link
Contributor Author

clrcrl commented Apr 5, 2021

Closing in favor of #327

@clrcrl clrcrl closed this Apr 5, 2021
clrcrl pushed a commit that referenced this pull request Apr 5, 2021
Taking jtcohen's approval on #318 as approval here ;)
clrcrl pushed a commit that referenced this pull request May 18, 2021
clrcrl pushed a commit that referenced this pull request May 18, 2021
clrcrl pushed a commit that referenced this pull request May 18, 2021
clrcrl pushed a commit that referenced this pull request May 18, 2021
@joellabes joellabes deleted the sequential-values branch October 21, 2021 22:33
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