add data synchronization test to verification Suite. #526

VenkataKarthikP · 2023-12-26T07:06:39Z

*Issue, if available: #501

Description of changes: Adding data synchronization check to verification suite, with this change users can define isDataSynchronized check.

Example usage -

val verificationResult = VerificationSuite()
  .onData(data)
  .addCheck(Check(CheckLevel.Error, "must have data in sync")
                                      .isDataSynchronized(dfToCompare, Map("id" -> "id"), _ > 0.7)
  .run()

cc: @mentekid @rdsharma26
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rdsharma26

Thank you @VenkataKarthikP for this PR! I've left some comments.

Note: We use this check for the DatasetMatch rule in AWS Glue Data Quality. In a future PR, we can rename DataSynchronization to DatasetMatch for consistency purposes.

src/main/scala/com/amazon/deequ/analyzers/Analyzer.scala

rdsharma26 · 2024-01-02T21:34:17Z

src/main/scala/com/amazon/deequ/analyzers/Analyzer.scala

+ * Data Synchronization Analyzer
+ *
+ * @param dfToCompare    DataFrame to compare
+ * @param columnMappings columns mappings


nit: We can add more documentation here. Are these the key column mappings? What about the other parameter, comparison column mappings? Will that be added in a future PR?

thanks, will update documentation.
I was planning to do this in phased manner instead of one big PR, will definitely do a follow up PR to add that as well.

src/main/scala/com/amazon/deequ/analyzers/Analyzer.scala

src/main/scala/com/amazon/deequ/comparison/ComparisonResult.scala

rdsharma26 · 2024-01-02T21:44:13Z

src/main/scala/com/amazon/deequ/comparison/ComparisonResult.scala

+case class ComparisonFailed(errorMessage: String, ratio: Double = 0) extends ComparisonResult
+case class ComparisonSucceeded(ratio: Double = 0) extends ComparisonResult


We should keep 1 set of states. Right now, there are 2 sets of states. Comparison[Failed/Succeeded] vs DataSynchronization[Failed/Succeeded]. Having too many states can result in confusion for the end user.

I was planning, to consolidate once ReferentialIntegrity is also integrated into verification suite. I will handle that in next PR.

rdsharma26 · 2024-01-02T21:44:52Z

src/main/scala/com/amazon/deequ/checks/Check.scala

-      .collect { case constraint: AnalysisBasedConstraint[_, _, _] => constraint.analyzer }
+      .collect {
+        case constraint: AnalysisBasedConstraint[_, _, _] => constraint.analyzer
+        case constraint: DataSynchronizationConstraint => constraint.analyzer


Why can't the existing statement match DataSynchronizationConstraint ?

updated, yes we don't need this.

src/main/scala/com/amazon/deequ/constraints/Constraint.scala

rdsharma26 · 2024-01-02T21:47:54Z

src/main/scala/com/amazon/deequ/constraints/Constraint.scala

+
+  override def evaluate(metrics: Map[Analyzer[_, Metric[_]], Metric[_]]): ConstraintResult = {
+
+    val anz = Try(metrics.filter(i => i._1.isInstanceOf[DataSynchronizationAnalyzer]).head._2)


Can we write it as the following, for better readability?

val (_, anz) = Try(metrics.filter { case(analyzer, _) => analyzer.isInstanceOf[DataSynchronizationAnalyzer] }.head)

What happens if .head is called on empty list? Is that possible?

will refactor to make it improve readability, thanks for suggestion.

VenkataKarthikP · 2024-01-04T10:38:01Z

@rdsharma26 thanks for the review, updated with review comments.

rdsharma26 · 2024-01-09T03:29:12Z

src/main/scala/com/amazon/deequ/analyzers/DataSynchronizationAnalyzer.scala

+ *
+ * @param dfToCompare The DataFrame to compare with the primary DataFrame that is setup
+ *                    during [[com.amazon.deequ.VerificationSuite.onData]] setup.
+ * @param columnMappings A map where each key-value pair represents a column in the primary DataFrame


Please rename to keyColumnMappings in next PR.

rdsharma26 · 2024-01-09T03:29:18Z

src/main/scala/com/amazon/deequ/analyzers/DataSynchronizationAnalyzer.scala

+ *
+ */
+case class DataSynchronizationAnalyzer(dfToCompare: DataFrame,
+                                       columnMappings: Map[String, String],


Please rename to keyColumnMappings in next PR.

rdsharma26

LGTM. Thanks again for the PR! Please address the following in next PR.

columnMappings should be renamed to keyColumnMappings
Add matchColumnMappings
[If possible] Change DataSynchronization to DatasetMatch

* add data synchronization test to verification suite * review comments * update test and doc strings

add data synchronization test to verification suite

6461856

rdsharma26 reviewed Jan 2, 2024

View reviewed changes

review comments

08a71ab

update test and doc strings

6c1f97e

VenkataKarthikP requested a review from rdsharma26 January 8, 2024 16:52

rdsharma26 reviewed Jan 9, 2024

View reviewed changes

rdsharma26 approved these changes Jan 9, 2024

View reviewed changes

rdsharma26 merged commit f3222c1 into awslabs:master Jan 9, 2024
1 check passed

VenkataKarthikP mentioned this pull request Jan 15, 2024

support col match and change to DatasetMatch #529

Merged

eycho-am pushed a commit that referenced this pull request Feb 21, 2024

add data synchronization test to verification Suite. (#526)

74f7a4b

* add data synchronization test to verification suite * review comments * update test and doc strings

eycho-am pushed a commit that referenced this pull request Feb 21, 2024

add data synchronization test to verification Suite. (#526)

4cf8ca6

* add data synchronization test to verification suite * review comments * update test and doc strings

rdsharma26 pushed a commit that referenced this pull request Apr 16, 2024

add data synchronization test to verification Suite. (#526)

372955f

* add data synchronization test to verification suite * review comments * update test and doc strings

rdsharma26 pushed a commit that referenced this pull request Apr 16, 2024

add data synchronization test to verification Suite. (#526)

4cc9503

* add data synchronization test to verification suite * review comments * update test and doc strings

rdsharma26 pushed a commit that referenced this pull request Apr 16, 2024

add data synchronization test to verification Suite. (#526)

3dbfd9b

* add data synchronization test to verification suite * review comments * update test and doc strings

rdsharma26 pushed a commit that referenced this pull request Apr 17, 2024

add data synchronization test to verification Suite. (#526)

caffc24

* add data synchronization test to verification suite * review comments * update test and doc strings

rdsharma26 pushed a commit that referenced this pull request Apr 17, 2024

add data synchronization test to verification Suite. (#526)

a008a3b

* add data synchronization test to verification suite * review comments * update test and doc strings

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add data synchronization test to verification Suite. #526

add data synchronization test to verification Suite. #526

VenkataKarthikP commented Dec 26, 2023 •

edited

Loading

rdsharma26 left a comment •

edited

Loading

rdsharma26 Jan 2, 2024

VenkataKarthikP Jan 4, 2024

rdsharma26 Jan 2, 2024

VenkataKarthikP Jan 4, 2024

rdsharma26 Jan 2, 2024

VenkataKarthikP Jan 4, 2024

rdsharma26 Jan 2, 2024

VenkataKarthikP Jan 4, 2024

VenkataKarthikP commented Jan 4, 2024

rdsharma26 Jan 9, 2024 •

edited

Loading

rdsharma26 Jan 9, 2024 •

edited

Loading

rdsharma26 left a comment •

edited

Loading

		case class ComparisonFailed(errorMessage: String, ratio: Double = 0) extends ComparisonResult
		case class ComparisonSucceeded(ratio: Double = 0) extends ComparisonResult


		override def evaluate(metrics: Map[Analyzer[_, Metric[_]], Metric[_]]): ConstraintResult = {

		val anz = Try(metrics.filter(i => i._1.isInstanceOf[DataSynchronizationAnalyzer]).head._2)

add data synchronization test to verification Suite. #526

add data synchronization test to verification Suite. #526

Conversation

VenkataKarthikP commented Dec 26, 2023 • edited Loading

rdsharma26 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenkataKarthikP commented Jan 4, 2024

rdsharma26 Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

rdsharma26 Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

rdsharma26 left a comment • edited Loading

Choose a reason for hiding this comment

VenkataKarthikP commented Dec 26, 2023 •

edited

Loading

rdsharma26 left a comment •

edited

Loading

rdsharma26 Jan 9, 2024 •

edited

Loading

rdsharma26 Jan 9, 2024 •

edited

Loading

rdsharma26 left a comment •

edited

Loading