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

[MAINTENANCE] GCP Integration Pipeline fix #6259

Merged
merged 11 commits into from
Nov 2, 2022

Conversation

Shinnnyshinshin
Copy link
Contributor

@Shinnnyshinshin Shinnnyshinshin commented Oct 31, 2022

Overview

  1. Renaming of test_expectations_cfe and test_expectations to v2 and v3 (follow up to [MAINTENANCE] Rename cfe to v3_api #6190)

  2. Adjusting V3 Expectations for expect_column_values_to_be_between_tz_informed

    What was the issue?

    • Comparison between STRING and DATETIME is not supported by bigquery
    E great_expectations.exceptions.exceptions.MetricResolutionError: 
    (google.cloud.bigquery.dbapi.exceptions.DatabaseError) 400 No matching signature for operator >= for 
    argument types: STRING, DATETIME. Supported signature: ANY >= ANY at [3:33]
  3. Adjusting V2 Expectations for expect_column_values_to_match_like_pattern and expect_column_values_to_not_match_like_pattern.

    What was the issue?

    • Like-pattern resolution in bigquery results in a NotImplementedError
    like_pattern_expression = self._get_dialect_like_pattern_expression(
            column, like_pattern, positive=False
        )
        if like_pattern_expression is None:
            logger.warning(
                f"Like patterns are not supported for dialect {str(self.sql_engine_dialect)}"
            )
     >           raise NotImplementedError
     E           NotImplementedError
    
    
  • This PR renames the files and suppresses the tests for bigquery

Definition of Done

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have run any local integration tests and made sure that nothing is broken.

@netlify
Copy link

netlify bot commented Oct 31, 2022

Deploy Preview for niobium-lead-7998 ready!

Name Link
🔨 Latest commit 1be0ce7
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/636294950b1a22000ad394fb
😎 Deploy Preview https://deploy-preview-6259--niobium-lead-7998.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@Shinnnyshinshin Shinnnyshinshin self-assigned this Oct 31, 2022
@ghost
Copy link

ghost commented Oct 31, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@Shinnnyshinshin Shinnnyshinshin changed the title [MAINTENANCE] GCP Integration Pipeline fix - Rename test file [MAINTENANCE] GCP Integration Pipeline fix Nov 1, 2022
@Shinnnyshinshin Shinnnyshinshin marked this pull request as ready for review November 1, 2022 16:49
Comment on lines -120 to -122
# The pip freeze output could be helpful to reproduce performance test results.
- publish: pip-freeze.txt
artifact: PipFreeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pipfreeze has not been used to reproduce test results, and is now giving errors in our pipeline.

ApplicationInsightsTelemetrySender correlated 2 events with X-TFS-Session 7df3e132-aada-4a43-b781-0a06fe1923db
##[error]Artifact PipFreeze already exists for build 119157.
Finishing: PublishPipelineArtifact

Copy link
Contributor

@kenwade4 kenwade4 left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor change request

azure-pipelines-gcp-integration.yml Outdated Show resolved Hide resolved
Copy link
Member

@anthonyburdi anthonyburdi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for this fix.

@Shinnnyshinshin Shinnnyshinshin merged commit ba3ee8f into develop Nov 2, 2022
@Shinnnyshinshin Shinnnyshinshin deleted the m/_/renaming-expections-file branch November 2, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants