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

Thresholds parsed as JSON floats are ignored #77

Open
colindean opened this issue Jan 20, 2022 · 1 comment
Open

Thresholds parsed as JSON floats are ignored #77

colindean opened this issue Jan 20, 2022 · 1 comment
Labels
bug Something isn't working

Comments

@colindean
Copy link
Collaborator

Describe the bug

When specifying a check with a threshold that will parse to a JSON float, e.g.

threshold: 0.10 # will be ignored
threshold: 10% # works
threshold: "0.10" # works

the threshold will be ignored.

To Reproduce

Configure a check with:

type: nullCheck
column: foo
threshold: 0.10

or put that into a test in NullCheckSpec.

Expected behavior

Thresholds specified as floats should work.

@colindean colindean added the bug Something isn't working label Jan 20, 2022
@colindean
Copy link
Collaborator Author

Workaround

The workaround for now is to use one of the two working forms: use the % sign version or ensure that it's passed as a string using quotes.

Analysis

I'm able to reproduce this in a modified test and am working on the solution. It's going to change a bunch of checks so it's not a quick fix.

The crux of the problem is here:

val threshold = c.downField("threshold").as[String].right.toOption

When DV's YAML parser converts to JSON internally, the threshold is represented as a float, so it needs .as[Float] there. The core problem is the discard of the Left[DecodingFailure] when the .as() fails.

DV's ConfigParserSpec doesn't fail when the thresholds are added because the checks inside of ValidatorTable are not checked in the equality assertion because ValidatorTable is an abstract class and thus its members, including the checks, aren't checked.

Impact

Since all checks parse their own config, we've gotta fix this across all checks with thresholds individually. This might be a time to centralize that configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant