Fix AnalyzerOptions in DQDL rule converters#688
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| (mc, col) => addWhereClause(rule, mc.isComplete(col)) | ||
| val multiColCheck = addWhereClause(rule, check.isPrimaryKey(head, None, opts, tail: _*)) | ||
| val withCompleteness = cols.foldLeft(multiColCheck) { | ||
| (mc, col) => addWhereClause(rule, mc.isComplete(col, None, opts)) |
There was a problem hiding this comment.
Bug: withCompleteness is computed but the Right(...) on line 52 uses withCompleteness, which is correct now. However, the original code had a bug where cols.foldLeft(multiColCheck) discarded the result (since Check is immutable and foldLeft returns a new value that was never assigned). The fix correctly captures the result in withCompleteness, but the addWhereClause wrapping inside the foldLeft body is redundant — addWhereClause is already applied via .where(...) on the isComplete call if needed. More importantly, the opts parameter is passed to check.isPrimaryKey(head, None, opts, tail: _*) but looking at the Check class, isPrimaryKey signature is isPrimaryKey(column: String, hint: Option[String], analyzerOptions: Option[AnalyzerOptions], columns: String*). Verify that this matches the actual method signature — if the method signature is isPrimaryKey(column: String, hint: Option[String], columns: String*) (without analyzerOptions), this will fail to compile.
| val numericOperands = rawOperands.collect { case a: AtomicNumberOperand => a.getOperand.toDouble } | ||
|
|
||
| val opts = analyzerOptionsForWhereClause(rule) | ||
| val nullFailOpts: Option[AnalyzerOptions] = |
There was a problem hiding this comment.
nullFailOpts is constructed using opts.map(_.filteredRow).getOrElse(FilteredRowOutcome.TRUE) — but when opts is None (no WHERE clause), this still produces NullBehavior.Fail with FilteredRowOutcome.TRUE. This means even without a WHERE clause, the EQUALS case uses NullBehavior.Fail. This is intentional per the PR description ("ColumnValues EQUALS to ignore NULLs instead of failing the assertion"), but the variable name and construction could be clearer. The opts variable is None when there's no WHERE clause, so opts.map(_.filteredRow) is None, and .getOrElse(FilteredRowOutcome.TRUE) gives TRUE. This is correct but fragile — if DEFAULT_ANALYZER_OPTION changes its filteredRow value, this won't track it.
| .hasEntropy(col, assertionAsScala(rule, rule.getCondition.asInstanceOf[NumberBasedCondition])) | ||
| Right( | ||
| addWhereClause(rule, check), | ||
| check, |
There was a problem hiding this comment.
Removing addWhereClause means that if a user specifies a WHERE clause on an Entropy rule (e.g., Entropy "col" > 0.5 where "grp = 'a'"), the WHERE clause is now silently ignored. This could be surprising to users. Consider either: (1) returning a Left(...) error if isWhereClausePresent(rule) to explicitly reject unsupported WHERE clauses, or (2) documenting this limitation clearly.
| val df = Seq( | ||
| (1, Some("")), (2, None), (3, Some("hello")) | ||
| ).toDF("id", "val") | ||
|
|
There was a problem hiding this comment.
This test asserts "Passed" for Entropy "val" > 0.5 where "grp = 'a'", but since the WHERE clause is now silently ignored (per the EntropyRule change), the entropy is computed over all 4 rows. With 2 distinct values equally distributed (x=2, y=2), entropy = -2*(0.5*ln(0.5)) ≈ 0.693, which is > 0.5, so it passes. However, the test comment says "ETL does not apply where clause for Entropy" — this is testing that the WHERE clause is ignored, not that it's applied. The test name should reflect this (e.g., "ignore where clause and compute over all rows"). More importantly, if a user expects the WHERE clause to filter, this silent ignore is a behavioral regression for anyone who previously relied on it.
|
This PR has been inactive for 60 days. It will be closed in 14 days if there is no further activity. If you are still working on this, please push an update or comment to keep it open. |
2 participants
Issue #, if available:
Description of changes:
This PR fixes how AnalyzerOptions are passed to Deequ Check methods in the DQDL rule converters, ensuring correct NULL handling when WHERE clauses are present.
Why was this necessary:
Without this fix, DQDL rule converters pass no
AnalyzerOptionsto Deequ Check methods, defaulting toNullBehavior.Ignore. This causes:WHEREclause rules to compute incorrect metrics (filtered NULLs silently ignored instead of treated as empty strings)WHEREclause is present (Entropy is a global metric)EMPTY/WHITESPACES_ONLYkeywords to produce SQL without NULL guards, causing incorrect compliance ratios when NULLs are presentChanges:
DQDLRuleConverter.scala: AddDEFAULT_ANALYZER_OPTIONconstant andanalyzerOptionsForWhereClause()helperCompletenessRule,IsCompleteRule,UniquenessRule,IsUniqueRule,UniqueValueRatioRule: Pass AnalyzerOptions when WHERE clause presentIsPrimaryKeyRule: Pass AnalyzerOptions + fix multi-column bug where completeness constraints were silently droppedColumnLengthRule: Pass AnalyzerOptions unconditionally (NULLs treated as length 0)ColumnValuesRule: Pass AnalyzerOptions for WHERE clause; useNullBehavior.Failfor EQUALS min/max; add NULL guards forEMPTYandWHITESPACES_ONLYkeywords in generated SQLEntropyRule: Remove WHERE clause filtering (global metric should not be filtered)ColumnValuesRuleSpec: Update 5 existing tests for new constraint representationsAnalyzerOptionParitySpec(new): 11 integration tests covering all changesAll 1020 tests pass.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.