[MinLength/MaxLength] Apply filtered row behavior at the row level evaluation #547

rdsharma26 · 2024-03-10T03:36:08Z

Description of changes:

For certain scenarios, the filtered row behavior for MinLength and MaxLength was not working correctly.
For example, when using both minLength and maxLength constraints in a single check, and with both using == as an assertion. This was resulting in the row level outcome of the filtered rows to be false. This was because we were replacing values for filtered rows for Min to MaxValue and for Max to MinValue. But a number could not equal both at the same time.

val analyzerOptions = AnalyzerOptions(
  nullBehavior = NullBehavior.EmptyString,
  filteredRow = FilteredRowOutcome.TRUE
)

val check = new Check(CheckLevel.Error, "test-check")
  .hasMinLength("Company", _ == 8, analyzerOptions = Some(analyzerOptions)).where("ID > 2")
  .hasMaxLength("Company", _ == 8, analyzerOptions = Some(analyzerOptions)).where("ID > 2")


+---+----------------+-------+-----+-----------+----------+
|ID |Company         |ZipCode|State|City       |test-check|
+---+----------------+-------+-----+-----------+----------+
|1  |Acme            |90210  |CA   |Los Angeles|false     |   <-- Incorrect outcome for filtered row
|2  |Acme            |90211  |CA   |Los Angeles|false     |   <-- Incorrect outcome for filtered row 
|3  |Robocorp        |NULL   |NJ   |NULL       |true      |
|4  |Robocorp        |NULL   |NY   |New York   |true      |
+---+----------------+-------+-----+-----------+----------+

Updated the logic of the row level assertion to MinLength/MaxLength to match what was done for Min/Max.

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

…aluation - For certain scenarios, the filtered row behavior for MinLength and MaxLength was not working correctly. - For example, when using both minLength and maxLength constraints in a single check, and with both using == <value> as an assertion. This was resulting in the row level outcome of the filtered rows to be false. This was because we were replacing values for filtered rows for Min to MaxValue and for Max to MinValue. But a number could not equal both at the same time. - Updated the logic of the row level assertion to MinLength/MaxLength to match what was done for Min/Max.

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

eycho-am · 2024-03-10T15:24:03Z

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

-        conditionSelectionGivenColumn(colLengths, Option(isNullCheck), replaceWith = 0.0).cast(DoubleType)
-      case _ =>
-        colLengths
+    val colLength = length(col(column)).cast(DoubleType)


nice, this is much cleaner

eycho-am

LGTM - added one small comment for documenting this new behavior

- Added comments to the "aggregationFunctions" method in Min, Max, MinLength and MaxLength analyzers. - Refactored the criterion method to reuse an existing variable.

rdsharma26 · 2024-03-10T20:20:22Z

Thanks for the review @eycho-am . I've addressed the comments in the latest commit.

…aluation (#547) * [MinLength/MaxLength] Apply filtered row behavior at the row level evaluation - For certain scenarios, the filtered row behavior for MinLength and MaxLength was not working correctly. - For example, when using both minLength and maxLength constraints in a single check, and with both using == <value> as an assertion. This was resulting in the row level outcome of the filtered rows to be false. This was because we were replacing values for filtered rows for Min to MaxValue and for Max to MinValue. But a number could not equal both at the same time. - Updated the logic of the row level assertion to MinLength/MaxLength to match what was done for Min/Max.

…aluation (awslabs#547) * [MinLength/MaxLength] Apply filtered row behavior at the row level evaluation - For certain scenarios, the filtered row behavior for MinLength and MaxLength was not working correctly. - For example, when using both minLength and maxLength constraints in a single check, and with both using == <value> as an assertion. This was resulting in the row level outcome of the filtered rows to be false. This was because we were replacing values for filtered rows for Min to MaxValue and for Max to MinValue. But a number could not equal both at the same time. - Updated the logic of the row level assertion to MinLength/MaxLength to match what was done for Min/Max.

eycho-am reviewed Mar 10, 2024

View reviewed changes

src/main/scala/com/amazon/deequ/analyzers/MaxLength.scala Show resolved Hide resolved

eycho-am reviewed Mar 10, 2024

View reviewed changes

eycho-am approved these changes Mar 10, 2024

View reviewed changes

Addressed feedback from PR review

7bb7086

- Added comments to the "aggregationFunctions" method in Min, Max, MinLength and MaxLength analyzers. - Refactored the criterion method to reuse an existing variable.

eycho-am approved these changes Mar 10, 2024

View reviewed changes

eycho-am merged commit 798901e into awslabs:master Mar 10, 2024
1 check passed

rdsharma26 deleted the minmax-length-row-level branch March 10, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MinLength/MaxLength] Apply filtered row behavior at the row level evaluation #547

[MinLength/MaxLength] Apply filtered row behavior at the row level evaluation #547

rdsharma26 commented Mar 10, 2024 •

edited

Loading

eycho-am Mar 10, 2024

eycho-am left a comment

rdsharma26 commented Mar 10, 2024

[MinLength/MaxLength] Apply filtered row behavior at the row level evaluation #547

[MinLength/MaxLength] Apply filtered row behavior at the row level evaluation #547

Conversation

rdsharma26 commented Mar 10, 2024 • edited Loading

eycho-am Mar 10, 2024

Choose a reason for hiding this comment

eycho-am left a comment

Choose a reason for hiding this comment

rdsharma26 commented Mar 10, 2024

rdsharma26 commented Mar 10, 2024 •

edited

Loading