Missing Column Precondition for Compliance Check - issue fix 467 #478
Conversation
Missing Column Precondition for Compliance Check - issue fix 467
…a test in Constraint Json string
|
Thanks for opening this PR, we'll take a look as soon as we can! |
Thank You @mentekid! |
| */ | ||
| case class Compliance(instance: String, predicate: String, where: Option[String] = None) | ||
| case class Compliance(instance: String, predicate: String, columns: List[String], where: Option[String] = None) |
There was a problem hiding this comment.
The new argument columns should be optional for backwards-compatibility, since users didn't have to specify it until now for the analyzer to work. Most people will only instantiate a Compliance object from a Check, so your code should still work as is, but since Compliance is a public class we should ensure it can be constructed without the column list.
There was a problem hiding this comment.
Also, could this be a trait? That way you don't have to implement additionalPreconditions here, you just rely on the trait (something like RequiresColumnsToExist) to define the preconditions, and other checks can inherit it effortlessly
More of a matter of opinion - if you see it works better as it is right now I have no objections.
There was a problem hiding this comment.
I tried to make it a trait, changes felt not future proof. Following the pattern of additionalPreconditions as this function can add additional checks later.
Example: one from completeness
I have made updates to make columns as Option and added the tests. Will update the PR once the internal review is complete.
|
It looks like the build is failing - can you take a look and address any issues? |
Yes, some lines are exceeding the column limit. Updating it now. |
| case class Compliance(instance: String, | ||
| predicate: String, | ||
| where: Option[String] = None, | ||
| columns: Option[List[String]] = None) |
There was a problem hiding this comment.
| columns: Option[List[String]] = None) | |
| columns: List[String] = List.empty[String]) |
There was a problem hiding this comment.
Then whole impl. will be a lot of easier
There was a problem hiding this comment.
Thank You @explicite!
I missed to see outside the box and followed other param's pattern, this would simplify it.
There was a problem hiding this comment.
Thanks @samarth-c1 for this contribution. This looks good to me, I kicked off the build and will merge if it passes.
And thanks @explicite for the List recommendation, I missed that.
* issue fix 467 * moved the test from AnalysisRunner to VerificaitionSuite, also fixed a test in Constraint Json string * remove unnecessary spacings * make the columns as optional and updated the test for isComplete for completenessCheck * alignment changes * use empty list as default instead of an option of a list
* issue fix 467 * moved the test from AnalysisRunner to VerificaitionSuite, also fixed a test in Constraint Json string * remove unnecessary spacings * make the columns as optional and updated the test for isComplete for completenessCheck * alignment changes * use empty list as default instead of an option of a list
* issue fix 467 * moved the test from AnalysisRunner to VerificaitionSuite, also fixed a test in Constraint Json string * remove unnecessary spacings * make the columns as optional and updated the test for isComplete for completenessCheck * alignment changes * use empty list as default instead of an option of a list
* issue fix 467 * moved the test from AnalysisRunner to VerificaitionSuite, also fixed a test in Constraint Json string * remove unnecessary spacings * make the columns as optional and updated the test for isComplete for completenessCheck * alignment changes * use empty list as default instead of an option of a list
Issue fix #467
Description of changes:
Added the
additionalPreconditionsfor Compliance constraint so the compliance check would fail and not spark operation. Used the columns check for the precondition to fail instead of letting it skipping the same. It was difficult to get the column names fromcolumnConditionhence used the new param to the function calls.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.