Copilot commented on code in PR #41856:
URL: https://github.com/apache/superset/pull/41856#discussion_r3539294041
##########
superset/semantic_layers/cache.py:
##########
@@ -541,9 +544,11 @@ def _implies(new: Filter, cached: Filter) -> bool: #
noqa: C901
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
Review Comment:
The new containment checks treat `IN` sets containing `None` as
“NULL-matching” and force `_implies` to return `False` via `all(v is not None
...)`. In SQL, `col IN (..., NULL, ...)` still never matches NULL rows in a
`WHERE` clause (it behaves like OR with UNKNOWN), so presence of `None` in the
list should not by itself break containment. This can cause `can_satisfy` to
reject otherwise-reusable cached entries (reduced cache hit rate / more
warehouse queries). A concrete fix is to ignore/drop `None` values from `nval`
for implication checks (and handle the “only-None” case as matching the empty
set), rather than rejecting the entire predicate whenever `None` appears.
##########
superset/semantic_layers/cache.py:
##########
@@ -723,7 +731,10 @@ def _mask_for(df: pd.DataFrame, f: Filter) -> pd.Series:
# noqa: C901
if op == Operator.EQUALS:
return series == val if val is not None else series.isna()
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()
Review Comment:
The inline conditional expression combined with `&` is correct but harder to
scan quickly. Consider rewriting this as an explicit `if val is None: ...;
else: ...` (or at least add parentheses around the conditional expression) to
make operator precedence unambiguous and reduce the chance of accidental
changes breaking the intended NULL semantics.
##########
superset/semantic_layers/cache.py:
##########
@@ -523,9 +523,12 @@ def _implies(new: Filter, cached: Filter) -> bool: #
noqa: C901
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
Review Comment:
The new containment checks treat `IN` sets containing `None` as
“NULL-matching” and force `_implies` to return `False` via `all(v is not None
...)`. In SQL, `col IN (..., NULL, ...)` still never matches NULL rows in a
`WHERE` clause (it behaves like OR with UNKNOWN), so presence of `None` in the
list should not by itself break containment. This can cause `can_satisfy` to
reject otherwise-reusable cached entries (reduced cache hit rate / more
warehouse queries). A concrete fix is to ignore/drop `None` values from `nval`
for implication checks (and handle the “only-None” case as matching the empty
set), rather than rejecting the entire predicate whenever `None` appears.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
