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

"isError" status instead of "failed" status #569

Open
dmitryilyn opened this issue Oct 15, 2024 · 3 comments
Open

"isError" status instead of "failed" status #569

dmitryilyn opened this issue Oct 15, 2024 · 3 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@dmitryilyn
Copy link
Contributor

It looks like if any check finishes with an error message, it will return isError = 1 instead of failed = 1:

if (!is.na(checkResults[i, ]$error)) {

This behavior breaks the cdmTable check and the cdmField check - they will both return isError instead of failed in case of missing table or field.

It also breaks the NA status calculation:

missingFields <- checkResults %>%

If I remember correctly, there was previously a parsing of the error message to determine which status to use - failed or isError.
If the error message was about a missing table or field, then failed status was used, otherwise isError status was used.
But this was not interpreted for all SQL dialects, and the default behavior for such cases was simply to set failed = 1 for any error.

@katy-sadowski katy-sadowski added the enhancement New feature or request label Oct 16, 2024
@katy-sadowski
Copy link
Collaborator

I agree this is unideal behavior and that we should refactor the way that cdmTable and cdmField failures/errors are handled. I think we need to add special handling for these types of checks (I would also like to add the option to query the information schema for users who have read access - this is really the best way to determine DDL conformance).

@dmitryilyn
Copy link
Contributor Author

I believe that special error handling in case of missing table or field should be applied to other checks as well.
For example: suppose a field is missing, the cdmField check will fail and return “failed” status, but when we try to run any other check for that field, it will return an error, and since the “isError” status has a higher priority than the “NA” status, the check will be marked with “isError” status instead of “NA” status.

# Errors precede all other statuses

@katy-sadowski
Copy link
Collaborator

Good flag; agree this should also be changed. Ideally, checks that will be NA should not even run in the first place! We've been discussing a refactor of the end to end DQD workflow and will definitely consider this as part of that work.

@katy-sadowski katy-sadowski added the bug Something isn't working label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants