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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] V3 implement expect_column_pair_values_to_be_in_set for SQL Alchemy execution engine #3281

Merged
merged 13 commits into from
Aug 20, 2021
Merged

[FEATURE] V3 implement expect_column_pair_values_to_be_in_set for SQL Alchemy execution engine #3281

merged 13 commits into from
Aug 20, 2021

Conversation

NathanFarmer
Copy link
Contributor

Changes proposed in this pull request:

  • JIRA: GREAT-56/GREAT-5

Definition of Done

Please delete options that are not relevant.

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added unit tests where applicable and made sure that new and existing tests are passing.
  • I have run any local integration tests and made sure that nothing is broken.

[FEATURE] GREAT-56/GREAT-5 V3 implement expect_column_pair_values_to_…
630217f 
…be_in_set for SQL Alchemy execution engine
@netlify
Copy link

netlify bot commented Aug 19, 2021 •
edited
Loading

✔️ Deploy Preview for niobium-lead-7998 ready!

🔨 Explore the source changes: a8d9f01

🔍 Inspect the deploy log: https://app.netlify.com/sites/niobium-lead-7998/deploys/611f01065cebf10007df7d7e

😎 Browse the preview: https://deploy-preview-3281--niobium-lead-7998.netlify.app

@NathanFarmer NathanFarmer self-assigned this Aug 19, 2021
@github-actions
Copy link
Contributor

HOWDY! This is your friendly 🤖 CHANGELOG bot 🤖

Please don't forget to add a clear and succinct description of your change under the Develop header in docs_rtd/changelog.rst, if applicable. This will help us with the release process. See the Contribution checklist in the Great Expectations documentation for the type of labels to use!

✨ Thank you! ✨

NathanFarmer
NathanFarmer commented Aug 19, 2021
View reviewed changes
tests/expectations/metrics/test_core.py Outdated

assert results[unexpected_values_metric.id][0] == (10, 1)
assert results[unexpected_values_metric_2.id] == []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shinnnyshinshin this is the vacuously true condition test you proposed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@NathanFarmer Do we need the various metrics_2? Since these are one-time use, can we reuse the original variable names? Also, some tests appear to be failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexsherstinsky We can reuse the variable names in the same test if that is preferred. I'm currently debugging the mssql issue.

Nathan Farmer added 2 commits August 19, 2021 09:24
[FEATURE] GREAT-56/GREAT-5 V3 enable integration tests for expect_col…
dce6df1 
…umn_pair_values_to_be_in_set for SQL Alchemy execution engine (#3281)
Update change log
2ad59c1
@NathanFarmer NathanFarmer marked this pull request as ready for review August 19, 2021 13:30
cdkini
cdkini approved these changes Aug 19, 2021
View reviewed changes
Copy link
Member

@cdkini cdkini left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

I would like @Shinnnyshinshin to see the assertion around the vacuously true condition before we merge (as long as this is non-blocking 😄)

alexsherstinsky
alexsherstinsky suggested changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@alexsherstinsky alexsherstinsky left a comment

Choose a reason for hiding this comment

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

A comment is inside.

@NathanFarmer NathanFarmer enabled auto-merge (squash) August 19, 2021 20:51
Nathan Farmer and others added 4 commits August 19, 2021 18:36
Linting
01cf28b
Merge branch 'feature/GREAT-56/GREAT-5/expect_column_pair_in_set_sqla…
250689c 
…lchemy' of github.com:great-expectations/great_expectations into feature/GREAT-56/GREAT-5/expect_column_pair_in_set_sqlalchemy
Debug
a1d5b6a
@NathanFarmer
Merge branch 'develop' into feature/GREAT-56/GREAT-5/expect_column_pa…
a8d9f01 
…ir_in_set_sqlalchemy
@NathanFarmer NathanFarmer merged commit 99a5437 into develop Aug 20, 2021
alexsherstinsky
alexsherstinsky approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@alexsherstinsky alexsherstinsky left a comment

Choose a reason for hiding this comment

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

@NathanFarmer Congratulations! All checks passed; not sure why it is not auto-merging...

@NathanFarmer NathanFarmer deleted the feature/GREAT-56/GREAT-5/expect_column_pair_in_set_sqlalchemy branch August 20, 2021 02:45
@alexsherstinsky
Copy link
Contributor

Oh, there we go! Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexsherstinsky alexsherstinsky alexsherstinsky approved these changes

@cdkini cdkini cdkini approved these changes

@jcampbell jcampbell Awaiting requested review from jcampbell

@fjork3 fjork3 Awaiting requested review from fjork3

@petermoyer petermoyer Awaiting requested review from petermoyer

@Shinnnyshinshin Shinnnyshinshin Awaiting requested review from Shinnnyshinshin

Assignees

@NathanFarmer NathanFarmer

Labels
core-team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NathanFarmer @alexsherstinsky @cdkini