fix(semantic-cache): NULL semantics in leftover masks + close the 100% coverage gap#41856
fix(semantic-cache): NULL semantics in leftover masks + close the 100% coverage gap#41856mikebridge wants to merge 2 commits into
Conversation
… in negative masks The unit-test CI job gates superset/semantic_layers/ at 100% coverage, and the branch was at 86.44% — its own CI red, and every stacked PR inheriting the red. This closes the gap to 100.00% (499 tests) with table-driven tests over the untested edges: - the pairwise filter-implication matrix (_implies and helpers): every IS_NULL / IS_NOT_NULL / EQUALS / NOT_EQUALS / IN / NOT_IN / range cross-combination, range-direction and comparability edges - can_satisfy edges: column-less WHERE rejection, redundant-vs-leftover filters on one column, ROLLUP order-on-dropped-dimension rejection, group-limit shape mismatch, projection gating - leftover-mask operator matrix, LIKE->regex translation, ordering with unknown columns, rollup with aggregation-less metrics - store_result's MAX_ENTRIES trim (oldest dropped first) - key/serialization helpers and the config timeout fallback - the full filter-value coercion matrix in mapper (every dtype's accept and reject branches) Writing the tests surfaced one REAL bug, fixed here: negative leftover masks (NOT_EQUALS / NOT_IN / NOT_LIKE) kept NULL rows, where SQL three-valued logic excludes them — cache-served results could contain rows the warehouse would have dropped. The masks now AND with notna(), and the operator matrix pins SQL semantics. Two provably-dead fallthroughs in _implies_range (both same-direction range-op combinations are exhaustively enumerated above them) carry justified no-cover pragmas rather than fabricated tests. Also flagged (not changed): _sql_like_to_regex has no escape-character support — LIKE '100\%' treats the percent as a wildcard, diverging from warehouses (e.g. PostgreSQL) where backslash escapes it. The test pins current behavior and the divergence is raised on the parent PR.
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## sl-cache #41856 +/- ##
============================================
+ Coverage 63.43% 64.27% +0.84%
============================================
Files 2592 2591 -1
Lines 138780 138644 -136
Branches 32235 32212 -23
============================================
+ Hits 88038 89120 +1082
+ Misses 49212 48003 -1209
+ Partials 1530 1521 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…mutant
Round-2 review panel on this PR (4 lenses; one ran 15 real mutants —
6 survived) plus its own findings, all resolved:
Production (completing the SQL three-valued-logic family):
1. Positive LIKE now excludes NULL rows: astype(str) stringifies NULLs
into "nan"/"None"/"NaT" sentinels that spuriously matched patterns
like 'n%' (empirically reproduced by two reviewers). SQL: NULL LIKE p
is UNKNOWN. Same guard the PR already applied to NOT_LIKE.
2. Positive IN now excludes NULL rows: pandas isin(None) matches None in
object columns; SQL IN-lists never match NULL rows even when the list
contains NULL.
3. _implies None-guards: a new NULL-matching predicate (EQUALS None, or
an IN set containing None) was wrongly treated as contained by cached
negative predicates (NOT_EQUALS / NOT_IN) — whose SQL result sets
contain no NULL rows — so a NULL-lookup query could be served an
EMPTY cached result. The IS_NOT_NULL branch already guarded this;
the negative branches now match.
Tests (mutation-hardening — every one of the 6 surviving mutants plus
the 4 new guards now has a killing row, re-validated one by one):
- upper-bound implication False rows (a True-mutant here means silently
truncated cache serves)
- the (LTE 5, LTE 5) row was theater — shadowed by the equality early
return; replaced with (LTE 4, LTE 5)
- the mask matrix DataFrame gains a NULL-bearing string row, so all
three LIKE-family notna() guards are observable; IN-with-NULL-element
row added
- LIKE regex end-anchor pinned ('a_c' rejects 'a.cd'); the no-escape
divergence is now pinned by a backslash row instead of only narrated
- the boolean token sets are fully enumerated (the set is the contract)
- None-hole implication rows for the new _implies guards
Hygiene per review: production imports hoisted to the module block,
planning-repo comment references replaced with the PR number, timeless
docstring wording.
Gate: 100.00%, 515 tests.
mikebridge
changed the title
|
Round-2 review (four-lens panel via Claude; one lens ran 15 real mutants against the suite — 6 survived; every claim below verified empirically) — all findings resolved in The 3VL NULL fix is now complete (round 1 fixed only the negative half):
Every surviving mutant now dies — re-validated one at a time: the upper-bound looser-containment mutants (which would serve silently truncated results), the shadowed Merge-coordination note for this stack (verified by merge replay): this PR and #41824 both append to Gate: 100.00%, 515 tests. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes SQL NULL-semantics mismatches in smart-cache leftover filtering and closes the superset/semantic_layers/ unit-test coverage gap to meet the 100% CI gate.
Changes:
- Fix NULL-handling for negative leftover masks (
NOT_EQUALS,NOT_IN,NOT_LIKE) and forIN/LIKEto better match SQL three-valued logic. - Add extensive table-driven unit tests covering filter implication, leftover mask behavior, LIKE→regex translation, cache trimming, and scalar filter-value coercion.
- Add tests for additional cache helper edges (ordering, rollup skips, key/serialization helpers, timeout fallback, and
can_satisfyrejection paths).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
superset/semantic_layers/cache.py |
Adjusts implication and mask generation logic to align with SQL NULL semantics and adds no-cover markers for unreachable branches. |
tests/unit_tests/semantic_layers/cache_test.py |
Adds broad matrix-style tests to close coverage gaps and validate cache reuse / masking behavior across operators and edge cases. |
tests/unit_tests/semantic_layers/mapper_test.py |
Adds coercion accept/reject matrices for scalar filter-value conversion by dimension dtype. |
| if cop == Operator.NOT_EQUALS: | ||
| if nop == Operator.NOT_EQUALS: | ||
| return nval == cval | ||
| if nop == Operator.EQUALS: | ||
| return nval != cval | ||
| # EQUALS None means IS_NULL; a cached negative predicate's rows | ||
| # contain no NULLs (SQL three-valued logic), so it can never | ||
| # contain the NULL-matching query. | ||
| return nval is not None and nval != cval | ||
| if nop == Operator.IN and isinstance(nval, frozenset): | ||
| return cval not in nval | ||
| return all(v is not None for v in nval) and cval not in nval | ||
| return False |
| if nop == Operator.NOT_EQUALS: | ||
| return cval.issubset({nval}) | ||
| if nop == Operator.EQUALS: | ||
| return nval not in cval | ||
| # See the NOT_EQUALS branch above: NULL-matching queries are | ||
| # never contained by negative cached predicates. | ||
| return nval is not None and nval not in cval | ||
| if nop == Operator.IN and isinstance(nval, frozenset): | ||
| return cval.isdisjoint(nval) | ||
| return all(v is not None for v in nval) and cval.isdisjoint(nval) | ||
| return False |
| if op == Operator.NOT_EQUALS: | ||
| return series != val if val is not None else series.notna() | ||
| # SQL three-valued logic: NULL != x is UNKNOWN, so the warehouse | ||
| # excludes NULL rows from a negative predicate; pandas would keep | ||
| # them (NaN != x is True). Match the warehouse. | ||
| return (series != val) & series.notna() if val is not None else series.notna() |
SUMMARY
Fourth contribution into #40221's branch (
sl-cache). The unit-test CI job gatessuperset/semantic_layers/at 100% coverage (--cov-fail-under=100), and the branch sits at 86.44% — its ownunit-tests (current)is red, and every stacked PR inherits the red regardless of its own coverage. This closes the gap to 100.00% (499 tests), which also directly addresses the long-standing patch-coverage review comment.Mostly table-driven tests over the untested edges: the full pairwise filter-implication matrix,
can_satisfyrejection edges, the leftover-mask operator matrix,LIKE→regex translation, theMAX_ENTRIEStrim, serialization helpers, and mapper's complete filter-value coercion matrix (every dtype's accept/reject branches).One real bug found and fixed (the point of testing these branches): negative leftover masks (
NOT_EQUALS/NOT_IN/NOT_LIKE) kept NULL rows — pandas evaluatesNaN != xas True, but SQL three-valued logic excludes NULLs from negative predicates. Cache-served results could contain rows the warehouse would have dropped. The masks now AND withnotna(), pinned by the operator matrix.Two findings flagged, not changed:
_sql_like_to_regexhas no escape-character support —LIKE '100\%'wildcards the%, diverging from warehouses (PostgreSQL escapes with backslash by default). The test pins current behavior; if escape support is added, extend the table._implies_range(both same-direction range-op combos are exhaustively enumerated above them) carry justified# pragma: no covercomments — deleting the dead lines is equally fine if preferred.TESTING INSTRUCTIONS
pytest --cov=superset/semantic_layers/ tests/unit_tests/semantic_layers/ --cov-fail-under=100 -q # 499 passed · Required test coverage of 100% reached. Total coverage: 100.00%Once this merges into
sl-cache, the branch's ownunit-tests (current)and every stacked PR's (#41824/#41825/#41826) go green.ADDITIONAL INFORMATION
🤖 Generated with Claude Code