Bugfixes 2.2.1#152
Open
mike-finopsorg wants to merge 22 commits into
Open
Conversation
…ng and entity type Object. (#132) Signed-off-by: Mike Fuller <mike@finops.org> --------- Signed-off-by: Mike Fuller <mike@finops.org>
Bumped version Signed-off-by: Mike Fuller <mike@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org> Signed-off-by: Mike Fuller <mike@finops.org>
Apply review feedback across the FOCUS 1.4 check generators: - Make every leaf generator row-condition-aware by wrapping its predicate_sql in _apply_condition, matching FormatJSON. Previously only FormatJSON did this; the predicate is read directly off the stored SQLQuery, so the condition must be baked in at construction. - Fix CheckColumnComparison null handling. The violation condition now requires both columns non-null before comparing, so null rows are no longer flagged. This aligns it with CheckSameValue's convention. - Collapse CheckGreaterOrEqual, CheckGreaterThan, and CheckLessOrEqual into a shared _CheckScalarComparisonGenerator base. Each subclass now sets only its operators, message phrase, and check type. This also standardizes literal quoting and predicate gating across the three. Tests: - Add a row-condition predicate test covering the named comparison generators to lock in the now-uniform behavior. - Update the CheckStringEndsWith assertion to expect the ends_with SQL. - Add tests/config_objects/test_generator_edge_cases.py, which executes generated SQL against a real in-memory DuckDB to cover null handling, multi-byte suffixes, malformed JSON paths, a missing jsonschema dependency, and row-filtered de-duplication. Signed-off-by: Mike Fuller <mike@finops.org>
Co-authored-by: Matt Cowsert <matthew@finops.org>
The Utf8 datetime parsing strategies each required a zero null_count, so a single null cell (an originally-null value or one unparseable entry) forced every strategy to fail and the column to stay a string. Nullable datetime columns are common in FOCUS, so this path was hit in practice. Judge parse success against the original null count instead of zero: a strategy now succeeds when parsing introduces no new nulls beyond those already present and the column has at least one real value. A nullable single-format column converts, a genuinely mixed or unparseable column is still rejected, and an all-null column is left as a string rather than coerced. Add regression tests covering nullable ISO (Z and offset), date-only, and space-separated columns, plus a mixed-format column that must be dropped. Also fix a stale TypeJSON test assertion: the generator now emits NOT json_valid(...) rather than typeof(...) != 'JSON'. Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Clean up issue with main having a squash marge on it Signed-off-by: Mike Fuller <mike@finops.org>
Corrections for issues found on PR: #147 --------- Signed-off-by: Mike Fuller <mike@finops.org>
…umnValue
ColumnByColumnEqualsColumnValueGenerator emitted ({a} * {b}) <> {r} with no
tolerance. The CSV loader infers numeric columns as Float64/DOUBLE, so the
product is computed in IEEE-754 and compared bit-exactly. Rows whose
exact-decimal product equals the result but whose float64 product differs by a
rounding bit (e.g. 0.000015 * 20 -> 0.00030000000000000003 vs 0.0003) were
wrongly flagged non-conformant.
Replace the exact comparison in both the violation and predicate SQL with a
relative epsilon via a shared _tolerance_expr helper so the two can't drift:
ABS(({a} * {b}) - {r}) <= 1e-9 * GREATEST(ABS({r}), 1). The tolerance absorbs
only float representation error, staying faithful to FOCUS exact-equality intent.
Resolves false failures for CAU-ListCost-C-011-C, CAU-ListUnitPrice-C-014-C,
CAU-ContractedCost-C-011-C, and CAU-ContractedUnitPrice-C-013-C.
Add end-to-end DuckDB regression tests and update the composite-calculation
generator test to assert the new tolerance SQL.
Signed-off-by: Mike Fuller <mike@finops.org>
Closed
Signed-off-by: Mike Fuller <mike@finops.org>
Signed-off-by: Mike Fuller <mike@finops.org>
Matt-Cowsert
left a comment
Matt-Cowsert
left a comment
Collaborator
There was a problem hiding this comment.
Thanks Mike, the fixes all check out. I ran the #144 repro and the full test suite against this branch locally (429 passing), since CI can't run here until the merge conflict is cleared.
Two housekeeping items:
The diff is mostly the already-merged 2.2.0 work. The real change vs main is about 120 lines: the float tolerance fix, the publish workflow fix, the version bump, and the new tests. A rebase onto main will collapse the diff to just that, clear the conflict, and let CI run. It will also drop the two commits missing sign-offs that have DCO stuck (one is the suggestion of mine you accepted; those don't get signed automatically).
Suggest dropping #149 from the fix list. That cycle comes from the 1.4 model's dependency declarations, and the actual fix is the FOCUS_Spec model change you filed (FinOps-Open-Cost-and-Usage-Spec/FOCUS_Spec#2536), not anything in this PR. Dropping it here keeps the release notes accurate.
One inline suggestion on the new tag patterns; everything else looks good.
Comment on lines
+5
to
+7
| - 'v[0-9].[0-9].[0-9]' | ||
| - 'v[0-9].[0-9].[0-9]-dev[0-9]' | ||
| - 'v[0-9].[0-9].[0-9]-rc[0-9]' |
Collaborator
There was a problem hiding this comment.
Suggested change
| - 'v[0-9].[0-9].[0-9]' | |
| - 'v[0-9].[0-9].[0-9]-dev[0-9]' | |
| - 'v[0-9].[0-9].[0-9]-rc[0-9]' | |
| - 'v[0-9]+.[0-9]+.[0-9]+' | |
| - 'v[0-9]+.[0-9]+.[0-9]+-dev[0-9]+' | |
| - 'v[0-9]+.[0-9]+.[0-9]+-rc[0-9]+' |
These patterns only match single-digit version parts, so the first v2.10.0 or v10.0.0 would silently skip publishing. GitHub's filter patterns support + (their docs use v[12].[0-9]+.[0-9]+ as an example). The create to push switch itself is the right fix for #153.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Bug fix list from raised issues: