Skip to content

Fix SQL value checks for negative expected values - #69893

Merged
potiuk merged 1 commit into
apache:mainfrom
aaron-y-chen:fix-sql-value-check-negative-tolerance
Jul 31, 2026
Merged

Fix SQL value checks for negative expected values#69893
potiuk merged 1 commit into
apache:mainfrom
aaron-y-chen:fix-sql-value-check-negative-tolerance

Conversation

@aaron-y-chen

@aaron-y-chen aaron-y-chen commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Hi team, I found a bug in SQLValueCheckOperator.

If numeric_pass_value_conv=-100 and self.tol=0.1, the resulting range is -90 <= record <= -110, which I don't think matches the expected behavior.


Was generative AI tooling used to co-author this PR?
  • No (please specify the tool below)

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@aaron-y-chen
aaron-y-chen marked this pull request as ready for review July 15, 2026 05:39
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 15, 2026

@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 a real bug, and the report pins it precisely. With pass_value=-100 and tol=0.1 the old expression produced:

-100 * (1 - 0.1) = -90     (lower)
-100 * (1 + 0.1) = -110    (upper)

so the check became -90 <= record <= -110 — unsatisfiable for any input. A negative pass_value with a tolerance could never pass, whatever the data said.

Using abs(v) * tol as a margin is the right fix, and it's a no-op for the positive case (v - v*tol is exactly v*(1-tol)), so there's no regression risk for existing users. Zero behaves identically too.

The part I'd single out as good judgement is extracting _get_tolerance_bounds and calling it from both _get_numeric_matches and the expected_str construction in _build_check_results. The old code duplicated the arithmetic in those two places, which is exactly how a displayed range drifts from the range actually enforced. Now they can't.

Worth knowing: #69736 fixes the same class of bug in the same file_get_match for SQLColumnCheckOperator/SQLTableCheckOperator around line 744, where check_values["geq_to"] * (1 - tolerance) moves the bound the wrong way for negatives. Different function, same root cause, and it reaches for the same abs(expected) * tolerance shape. They're far enough apart in the file that they shouldn't conflict, but you two may want to compare notes so the two fixes stay consistent.


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

@potiuk
potiuk merged commit 1c1f574 into apache:main Jul 31, 2026
204 checks passed
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.

2 participants