[MAINTENANCE] DX-565 remove allow_cross_type_comparison from column_pair_values.a_greater_than_b #8025

TrangPham · 2023-05-30T17:54:24Z

Metric column_pair_values.a_greater_than_b does not accept the allow_cross_type_comparison kwarg, but it was still listed on the expectation docstring.

Description of PR changes above includes a link to an existing GitHub issue
PR title is prefixed with one of: [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], [CONTRIB]
Code is linted
```
black .

ruff . --fix
```
Appropriate tests and docs have been updated

For more details, see our Contribution Checklist, Coding style guide, and Documentation style guide.

After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!

…pair_values.a_greater_than_b

netlify · 2023-05-30T17:54:29Z

✅ Deploy Preview for niobium-lead-7998 ready!

Name	Link
🔨 Latest commit	`950a095`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/6495eae3c77f05000851ed71
😎 Deploy Preview	https://deploy-preview-8025--niobium-lead-7998.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

ghost · 2023-05-30T17:55:51Z

👇 Click on the image for a new way to code review

Legend

anthonyburdi

Thank you! 🙇

alexsherstinsky

@TrangPham & @anthonyburdi Please bear with me for a moment. This may not be enough. Thanks.

alexsherstinsky · 2023-05-30T19:41:30Z

@TrangPham @anthonyburdi -- right now, the following modules in the V3 and higher versions of GX OSS (including ones in the "expectations", "metrics", and "tests" subdirectories) deal with the allow_cross_type_comparisons directive in one way or another:

great_expectations/core/expectation_configuration.py
great_expectations/expectations/core/expect_column_pair_values_a_to_be_greater_than_b.py
great_expectations/expectations/core/expect_column_values_to_be_between.py
great_expectations/expectations/metrics/column_map_metrics/column_values_between.py
great_expectations/expectations/metrics/column_pair_map_metrics/column_pair_values_greater.py
great_expectations/jupyter_ux/expectation_explorer.py
great_expectations/self_check/util.py

Is what is being proposed in the present pull request the most optimal handling of the deprecation of allow_cross_type_comparisons? It seems that we should look at at least one additional expectation and possibly elsewhere as well.

Thank you.

…alues a to b

TrangPham · 2023-05-30T19:56:19Z

great_expectations/core/expectation_configuration.py
great_expectations/expectations/core/expect_column_pair_values_a_to_be_greater_than_b.py
great_expectations/expectations/core/expect_column_values_to_be_between.py
great_expectations/expectations/metrics/column_map_metrics/column_values_between.py
great_expectations/expectations/metrics/column_pair_map_metrics/column_pair_values_greater.py
great_expectations/jupyter_ux/expectation_explorer.py
great_expectations/self_check/util.py

expect_column_values_to_be_between allows allow_cross_type_comparison to be passed in. I have removed more references to allow_cross_type_comparison for expect_column_pair_values_a_to_be_greater_than_b

great_expectations/dataset/dataset.py

TrangPham · 2023-06-01T21:28:30Z

@alexsherstinsky Brought up some good questions in relation to this change:

Should we be deprecating allow_cross_type_comparison in one place but not others?
allow_cross_type_comparison was originally implemented to handle string date to date comparisons, what is the new way of doing this if we are deprecating this option?

cc: @talagluck

TrangPham · 2023-06-05T17:20:38Z

@alexsherstinsky Here's the note from the changelog in 0.13.0

great_expectations/docs_rtd/changelog.rst

Line 3824 in 4beb6d1

    
           * BREAKING: The new Modular Expectations API will begin removing the ``parse_strings_as_datetimes`` and ``allow_cross_type_comparisons`` flags in expectations. Expectation Suites that use the flags will need to be updated to use the new Modular Expectations. In general, simply removing the flag will produce correct behavior; if you still want the exact same semantics, you should ensure your raw data already has typed datetime objects.

It seems that we should be removing and deprecating these params from all places.
This was from @jcampbell 3 years ago.

I think we can merge this and do a followup PR for removing the params from expect_column_values_to_be_between

jcampbell · 2023-06-06T12:40:08Z

@alexsherstinsky Here's the note from the changelog in 0.13.0

great_expectations/docs_rtd/changelog.rst

Line 3824 in 4beb6d1

* BREAKING: The new Modular Expectations API will begin removing the ``parse_strings_as_datetimes`` and ``allow_cross_type_comparisons`` flags in expectations. Expectation Suites that use the flags will need to be updated to use the new Modular Expectations. In general, simply removing the flag will produce correct behavior; if you still want the exact same semantics, you should ensure your raw data already has typed datetime objects.

It seems that we should be removing and deprecating these params from all places. This was from @jcampbell 3 years ago.

I think we can merge this and do a followup PR for removing the params from expect_column_values_to_be_between

Confirming that removing those makes sense to me!

alexsherstinsky · 2023-06-06T16:31:10Z

@alexsherstinsky Here's the note from the changelog in 0.13.0

great_expectations/docs_rtd/changelog.rst

Line 3824 in 4beb6d1

* BREAKING: The new Modular Expectations API will begin removing the ``parse_strings_as_datetimes`` and ``allow_cross_type_comparisons`` flags in expectations. Expectation Suites that use the flags will need to be updated to use the new Modular Expectations. In general, simply removing the flag will produce correct behavior; if you still want the exact same semantics, you should ensure your raw data already has typed datetime objects.

It seems that we should be removing and deprecating these params from all places. This was from @jcampbell 3 years ago.
I think we can merge this and do a followup PR for removing the params from expect_column_values_to_be_between

Confirming that removing those makes sense to me!

Then we should do it "wholesale codebase-wide" in a single pull-request (this way this change will be clear and revertible). Thanks!

…be_between

TrangPham · 2023-06-07T21:42:10Z

@jcampbell @alexsherstinsky I also removed it from expect_column_values_to_be_between. Please let me know if it needs to be further removed

…omparisons

TrangPham · 2023-06-20T21:31:58Z

@jcampbell @alexsherstinsky I fixed the tests. Can you re-review? Thanks!

jcampbell

Thank you!

alexsherstinsky

LGTM!

…omparisons

[MAINTAINANCE] DX-565 remove allow_cross_type_comparison from column_…

5a648db

…pair_values.a_greater_than_b

TrangPham requested a review from a team May 30, 2023 17:54

github-actions bot added the core label May 30, 2023

anthonyburdi approved these changes May 30, 2023

View reviewed changes

alexsherstinsky suggested changes May 30, 2023

View reviewed changes

Remove more instances of allow_cross_type_comparison on column pair v…

db73cca

…alues a to b

alexsherstinsky reviewed May 30, 2023

View reviewed changes

great_expectations/dataset/dataset.py Outdated Show resolved Hide resolved

Don't change v2 code

0be3bf3

Also remove allow_cross_type_comparison from expect_column_values_to_…

05b7001

…be_between

TrangPham and others added 5 commits June 16, 2023 09:54

Merge branch 'develop' into DX-565-update-docs-for-allow-cross-type-c…

91d4e75

…omparisons

allow_cross_type_comparision should error with NotImplementedError

1502382

ruff

cd60af2

Need to add catch_exceptions to tests

cbef6e4

Merge branch 'develop' into DX-565-update-docs-for-allow-cross-type-c…

f210d11

…omparisons

TrangPham requested a review from alexsherstinsky June 20, 2023 21:32

jcampbell approved these changes Jun 21, 2023

View reviewed changes

alexsherstinsky approved these changes Jun 21, 2023

View reviewed changes

TrangPham added 2 commits June 21, 2023 09:38

Merge branch 'develop' into DX-565-update-docs-for-allow-cross-type-c…

3a3d32d

…omparisons

Merge branch 'develop' into DX-565-update-docs-for-allow-cross-type-c…

e8d6bbb

…omparisons

TrangPham enabled auto-merge (squash) June 23, 2023 18:56

Merge branch 'develop' into DX-565-update-docs-for-allow-cross-type-c…

950a095

…omparisons

TrangPham merged commit b914b62 into develop Jun 23, 2023
45 checks passed

TrangPham deleted the DX-565-update-docs-for-allow-cross-type-comparisons branch June 23, 2023 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINTENANCE] DX-565 remove allow_cross_type_comparison from column_pair_values.a_greater_than_b #8025

[MAINTENANCE] DX-565 remove allow_cross_type_comparison from column_pair_values.a_greater_than_b #8025

TrangPham commented May 30, 2023

netlify bot commented May 30, 2023 •

edited

ghost commented May 30, 2023 •

edited by ghost

Legend

anthonyburdi left a comment

alexsherstinsky left a comment

alexsherstinsky commented May 30, 2023 •

edited

TrangPham commented May 30, 2023

TrangPham commented Jun 1, 2023

TrangPham commented Jun 5, 2023

jcampbell commented Jun 6, 2023

alexsherstinsky commented Jun 6, 2023

TrangPham commented Jun 7, 2023

TrangPham commented Jun 20, 2023

jcampbell left a comment

alexsherstinsky left a comment

[MAINTENANCE] DX-565 remove allow_cross_type_comparison from column_pair_values.a_greater_than_b #8025

[MAINTENANCE] DX-565 remove allow_cross_type_comparison from column_pair_values.a_greater_than_b #8025

Conversation

TrangPham commented May 30, 2023

netlify bot commented May 30, 2023 • edited

✅ Deploy Preview for niobium-lead-7998 ready!

ghost commented May 30, 2023 • edited by ghost

Legend

anthonyburdi left a comment

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

alexsherstinsky commented May 30, 2023 • edited

TrangPham commented May 30, 2023

TrangPham commented Jun 1, 2023

TrangPham commented Jun 5, 2023

jcampbell commented Jun 6, 2023

alexsherstinsky commented Jun 6, 2023

TrangPham commented Jun 7, 2023

TrangPham commented Jun 20, 2023

jcampbell left a comment

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

netlify bot commented May 30, 2023 •

edited

ghost commented May 30, 2023 •

edited by ghost

alexsherstinsky commented May 30, 2023 •

edited