Skip to content

Fix SQL check tolerance for negative expected values - #69736

Merged
potiuk merged 1 commit into
apache:mainfrom
steveahnahn:fix-sql-check-tolerance-negative-thresholds
Jul 31, 2026
Merged

Fix SQL check tolerance for negative expected values#69736
potiuk merged 1 commit into
apache:mainfrom
steveahnahn:fix-sql-check-tolerance-negative-thresholds

Conversation

@steveahnahn

Copy link
Copy Markdown
Contributor

Split out of #69675 per review feedback there (the change is unrelated to that PR's asset-event topic).

SQLColumnCheckOperator / SQLTableCheckOperator tolerance scaled check bounds by (1 ± tolerance), which moves the bound the wrong way when the expected value is negative: with geq_to=-1000 and tolerance=0.1 the bound became -900, so a record equal to the threshold (-1000) failed the check, and equal_to=-100 produced an empty acceptance band ([-90, -110]). The fix computes the margin from the magnitude (abs(expected) * tolerance), so tolerance always widens the bound outward. Bounds for positive expected values are unchanged.

Adds parametrized regression tests covering negative thresholds for all five comparators, unchanged positive-threshold behavior, and the no-tolerance exact path.

related: #69675


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Fable 5)

Generated-by: Claude Code (Fable 5) following the guidelines

@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 11, 2026
@steveahnahn
steveahnahn force-pushed the fix-sql-check-tolerance-negative-thresholds branch from 409be8e to 35e4541 Compare July 11, 2026 19:02

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is the sibling of the bug just fixed in #69893 (now merged), and the same reasoning applies: scaling a bound by (1 ± tolerance) moves it the wrong way when the expected value is negative. For geq_to: -1000 with tolerance=0.1, the old code required record >= -900 — tightening the threshold instead of widening it, so values inside the intended tolerance band were rejected.

Two things I liked beyond the fix itself:

The refactor is a real simplification, not churn. Folding tolerance is None into _margin() returning 0 collapses five if/else pairs into single expressions — 35 lines to 27 — and the no-tolerance path stays exactly equivalent (record >= v - 0 is record >= v).

The tests are the most thorough in this batch. Parametrising over geq_to/greater_than/leq_to/less_than/equal_to with negative thresholds, plus the strict-vs-non-strict boundary case (greater_than: -1000, record: -1100 -> False), plus positive-threshold cases proving no regression, is exactly the coverage this kind of arithmetic fix needs.

One observation, not blocking: the equal_to branch changes from record == expected to expected - margin <= record <= expected + margin even when tolerance is None. For numerics that's equivalent, and the docs describe equal_to as an exact numeric value, so I don't think there's a real case at risk — just noting the no-tolerance path was widened slightly beyond what the fix required.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit 204ba78 into apache:main Jul 31, 2026
105 checks passed
@shahar1

shahar1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@potiuk please note the regression I fixed in #70895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants