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

refactor: custom query code #607

Merged
merged 23 commits into from
Oct 31, 2022
Merged

Conversation

pshrivastava27
Copy link
Contributor

This PR includes following changes:

  1. Refactoring custom query (row and column validation) code to use ibis framework (fix Refactor custom query validation to use query builder #539 )
    Instead of constructing nested queries manually using string concatenation, we now use ibis framework similar to how we construct queries for regular validations.
  2. Removing table list dependency for custom query row validation (fix Deprecate the usage of --tables-list/-tbls argument from DVT command for custom-query validation  #575 )
  3. Resolving issues specific to TD custom query row validation (trailing whitespaces after hash value)
  4. Making flags source-query-file and target-query-file mandatory for custom-query validation commands.
  5. Making primary-keysflag mandatory for custom-query row validation.

@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27
Copy link
Contributor Author

/gcbrun

@Raniksingh
Copy link
Contributor

Can you also update the README with the following:

  1. Remove tbls from custom query syntax(as this PR will remove tbls dependency from custom query
  2. add PK in the syntax for custom query as its a mandatory argument now
  3. add the syntax for concat option for both regular and custom query row validation syntax.

data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Show resolved Hide resolved
Copy link
Collaborator

@nehanene15 nehanene15 left a comment

Choose a reason for hiding this comment

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

Have we tested with saving the custom query validation to YAML and then running it?
Does this support comparison field validations or only hash/concat?

data_validation/cli_tools.py Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/validation_builder.py Show resolved Hide resolved
data_validation/query_builder/query_builder.py Outdated Show resolved Hide resolved
data_validation/config_manager.py Show resolved Hide resolved
data_validation/config_manager.py Outdated Show resolved Hide resolved
@pshrivastava27
Copy link
Contributor Author

Have we tested with saving the custom query validation to YAML and then running it? Does this support comparison field validations or only hash/concat?

Yes. We tested generating the yaml config and then running the validation using config file. Works as expected.

@pshrivastava27
Copy link
Contributor Author

Can you also update the README with the following:

  1. Remove tbls from custom query syntax(as this PR will remove tbls dependency from custom query
  2. add PK in the syntax for custom query as its a mandatory argument now
  3. add the syntax for concat option for both regular and custom query row validation syntax.

Done here

@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27 pshrivastava27 self-assigned this Oct 20, 2022
@pshrivastava27
Copy link
Contributor Author

/gcbrun

data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Show resolved Hide resolved
data_validation/validation_builder.py Outdated Show resolved Hide resolved
data_validation/validation_builder.py Outdated Show resolved Hide resolved
@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27
Copy link
Contributor Author

/gcbrun

data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/cli_tools.py Outdated Show resolved Hide resolved
data_validation/validation_builder.py Outdated Show resolved Hide resolved
data_validation/validation_builder.py Outdated Show resolved Hide resolved
data_validation/validation_builder.py Outdated Show resolved Hide resolved
tests/unit/test_validation_builder.py Outdated Show resolved Hide resolved
@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27
Copy link
Contributor Author

/gcbrun

Copy link
Collaborator

@nehanene15 nehanene15 left a comment

Choose a reason for hiding this comment

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

Logic looks good, just suggested some code style changes.

data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
data_validation/__main__.py Outdated Show resolved Hide resolved
@pshrivastava27
Copy link
Contributor Author

/gcbrun

Copy link
Collaborator

@nehanene15 nehanene15 left a comment

Choose a reason for hiding this comment

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

Approved. Suggested one last comment to delete the query_type in the arg parser since it is not used.

data_validation/cli_tools.py Outdated Show resolved Hide resolved
data_validation/cli_tools.py Outdated Show resolved Hide resolved
@pshrivastava27
Copy link
Contributor Author

/gcbrun

@pshrivastava27 pshrivastava27 enabled auto-merge (squash) October 31, 2022 12:39
@pshrivastava27 pshrivastava27 removed the request for review from Raniksingh October 31, 2022 12:41
@pshrivastava27 pshrivastava27 merged commit a7f800d into develop Oct 31, 2022
@pshrivastava27 pshrivastava27 deleted the custom-query-refactor-code branch October 31, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants