[Min/Max] Apply filtered row behavior at the row level evaluation #543
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows.
eycho-am
reviewed
Mar 6, 2024
| case FilteredRowOutcome.TRUE => true | ||
| case FilteredRowOutcome.NULL => null | ||
| } | ||
| case None => null |
There was a problem hiding this comment.
We had discussed that the default analyzerOptions behavior should be FilteredRowOutcome.TRUE, should we modify 933 to be true? (By default filtered rows are true instead of null.
There was a problem hiding this comment.
I was testing out another scenario that may be problematic.
Given the following dataframe:
+------+----+----+----+----+-----+-----+
|item |att1|att2|val1|val2|rule4|rule5|
+------+----+----+----+----+-----+-----+
|1 |a |f |1 |1 |true |true |
|22 |b |d |2 |NULL|true |true |
|333 |a |NULL|3 |3 |true |true |
|4444 |a |f |4 |4 |true |true |
|55555 |b |NULL|5 |NULL|true |true |
|666666|a |f |6 |6 |true |true |
+------+----+----+----+----+-----+-----+
where
val analyzerOptions = Option(AnalyzerOptions(filteredRow = FilteredRowOutcome.TRUE))
val min = new Check(CheckLevel.Error, "rule4")
.hasMin("val2", _ > 0, None, analyzerOptions)
.where("val1 < 4")
val max = new Check(CheckLevel.Error, "rule5")
.hasMax("val2", _ < 4, None, analyzerOptions)
.where("val1 < 4")
You'll see that rows 1,2,3 should be skipped -> True
Row 5 should be null as val2 is a null value there.
However, with the above method we convert all nulls to true/null - this doesn't distinguish between null values due to being filtered or null values due to null column values.
There was a problem hiding this comment.
Thanks @eycho-am for the valuable feedback. The latest PR revision contains a new structure for the column that helps maintain the "source" of a row, whether it is in scope and filtered out. That will help in evaluating the correct outcome for each row.
eycho-am
reviewed
Mar 6, 2024
| @@ -374,11 +371,11 @@ class VerificationSuiteTest extends WordSpec with Matchers with SparkContextSpec | |||
|
|
|||
| // filtered rows 1, 2, 3 (where item > 3) | |||
| val minRowLevel = resultData.select(expectedColumn4).collect().map(r => r.getAs[Any](0)) | |||
| assert(Seq(true, true, true, true, true, true).sameElements(minRowLevel)) | |||
| assert(Seq(null, null, null, true, true, true).sameElements(minRowLevel)) | |||
There was a problem hiding this comment.
these test were written with the intention that without specifying analyzer options, the default behavior would be filtered rows are true - related to above comment.
There was a problem hiding this comment.
Reverted the change.
- Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally.
eycho-am
reviewed
Mar 7, 2024
| case FilteredRowOutcome.TRUE => true | ||
| case FilteredRowOutcome.NULL => null | ||
| } | ||
| case None => null |
There was a problem hiding this comment.
nit: similar comment for the below test-case, should the default behavior for filtered rows be true? Or are we making a decision here that they'll be null by default?
Looks good to me otherwise, I think we just need to make final a decision on this point
There was a problem hiding this comment.
We are keeping them as is, unless the analyzer option says otherwise.
There was a problem hiding this comment.
Based on offline discussion, updated the logic. Thanks @eycho-am for the feedback.
We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior.
Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Mar 8, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Mar 8, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Apr 16, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Apr 16, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Apr 16, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Apr 17, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
rdsharma26
added a commit
that referenced
this pull request
Apr 17, 2024
* [Min/Max] Apply filtered row behavior at the row level evaluation - This changes from applying the behavior at the analyzer level. It allows us to prevent the usage of MinValue/MaxValue as placeholder values for filtered rows. * Improved the separation of null rows, based on their source - Whether the outcome for a row is null because of being filtered out or due to the target column being null, is now stored in the outcome column itself. - We could have reused the placeholder value to find out if a row was originally filtered out, but that would not work if the actual value in the row was the same originally. * Mark filtered rows as true We recently fixed the outcome of filtered rows and made them default to true instead of false, which was a bug earlier. This change maintains that behavior. * Added null behavior - empty string to match block Not having it can cause match error.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.