-
Notifications
You must be signed in to change notification settings - Fork 533
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columns: Option[List[String]] = None) | |
columns: List[String] = List.empty[String]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then whole impl. will be a lot of easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for List
is better to use empty list instead of boxing it in to Option
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
additionalPreconditions
for 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 fromcolumnCondition
hence 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.