fix: restore column-vs-literal comparison in isGreaterThan/isLessThan family (#227) - #273
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Ready for review. Restores column-vs-literal comparison across the |
50fc96c to
83c28e2
Compare
|
Thanks for the automated review pass. The current revision addresses these findings:
Resolving the threads accordingly. |
|
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. |
83c28e2 to
8779b30
Compare
|
rebased onto the latest master — still green and mergeable. this one restores the long-supported column-vs-literal usage in the isGreaterThan/isLessThan family (regressed in deequ 2.0.x), with tests. still relevant on my end and i'll keep it current; a review would be appreciated whenever there's bandwidth. |
| constraint_name, | ||
| assertion_func, | ||
| hint, | ||
| self._jvm.scala.collection.Seq.empty(), |
There was a problem hiding this comment.
Native passes columns = List(columnA, columnB) here, which flows through satisfies into complianceConstraint and on to the Compliance analyzer. Empty drops that. For the literal case [columnA] is right.
There was a problem hiding this comment.
Good catch — fixed. columns now gets List(columnA) instead of an empty Seq, so Compliance.preconditions runs hasColumn again; a misspelled column comes back as Input data does not include column X! rather than a Spark parse error (verified end to end through a VerificationSuite).
Left columnB out deliberately, per your note about the literal case: it may be a column, a literal or an expression, and the only way to tell from Python is a bare-identifier heuristic, which would read TRUE/NULL/CURRENT_DATE as columns and reintroduce the exact literal break this PR fixes. Happy to add an explicit opt-in arg if you want full column-vs-column parity with the native path.
One thing that fell out of this: the old scala.collection.Seq.empty() does not exist on Scala 2.13, so this also unblocks the Spark 4.1 matrix entry. Rebased onto master to pick that up — test_checks.py is 92 passed on both pyspark 3.5 (Scala 2.12.18) and 4.1.2 (Scala 2.13.17).
Unrelated, but worth a separate issue: scala_utils.to_scala_seq uses scala.collection.JavaConversions, which was also removed in 2.13, and analyzers.py calls it in several places.
…ly (awslabs#227) Deequ 2.0.x's Check.isLessThan/isLessThanOrEqualTo/isGreaterThan/ isGreaterThanOrEqualTo forward columns = List(columnA, columnB) to satisfies, which makes Deequ require both operands to be existing columns. This regressed the long-supported column-vs-literal usage (e.g. isGreaterThanOrEqualTo("cluster_size", "1")), failing with 'Input data does not include column 1!' (issue awslabs#227). Route the comparator family through Deequ's satisfies with an empty columns list (the pre-2.0 behaviour), building the SQL predicate in the wrapper. columnB may now be a column name or a SQL literal/expression. Column-vs-column comparisons are unchanged. Adds regression tests for column-vs-literal comparisons.
satisfies forwards columns into complianceConstraint and on to the Compliance analyzer, which runs columns.map(hasColumn) as a precondition. Passing an empty Seq skipped that check, so a misspelled columnA surfaced as a Spark SQL parse error rather than "Input data does not include column X!". Only columnA is listed: columnB may be a column, a literal or a SQL expression, and a bare-identifier heuristic would misread TRUE, NULL or CURRENT_DATE as columns and reintroduce the literal-comparison break. Building the list via genericWrapArray(...).toList() also drops scala.collection.Seq.empty(), which does not exist on Scala 2.13.
2445611 to
673844c
Compare
2 participants
Problem
isGreaterThanOrEqualTo(and the rest of the comparator family) regressed for column-vs-literal comparisons. This used to work:but now fails with the constraint message
Input data does not include column 1!— the second operand is interpreted strictly as a column name. Reported in #227.Root cause
This rode in with the bundled Deequ jar upgrade to 2.0.x (PyDeequ is pinned to
com.amazon.deequ:deequ:2.0.8-...viapydeequ/configs.py). In Deequ 1.2.x the comparators built a plain SQL predicate ("<colA> >= <colB>") and passed nocolumnslist, so a literal second operand worked. In Deequ 2.0.x the comparators passcolumns = List(columnA, columnB), and Deequ validates that every entry is a real dataframe column — so literals fail. The PyDeequ wrappers themselves never changed; they just forward to the regressed Scala methods.Fix
Route the whole comparator family (
isLessThan,isLessThanOrEqualTo,isGreaterThan,isGreaterThanOrEqualTo) through Deequ's publicsatisfies(...)with an emptycolumnslist — exactly the pre-2.0 behavior. Column-vs-column comparisons are unchanged; column-vs-literal/expression works again. Applied across the entire family for consistency.Note: as with Deequ's own
satisfies,columnBis treated as a Spark SQL expression, so string literals must be quoted by the caller (e.g."'foo'"). This matches the original 1.2.x semantics.Tests
Added to
tests/test_checks.py:test_comparator_against_literal— column-vs-literal for all four comparators, expecting Success.test_fail_comparator_against_literal— a failing literal comparison, expecting Failure.Validated against real Spark 3.5 / Deequ 2.0.8: the 2 new tests plus all 8 existing column-vs-column comparator tests pass (10 passed). CI will exercise the full pyspark 3.1/3.2/3.3/3.5 matrix.
Closes #227