bito-code-review[bot] commented on code in PR #44662:
URL: https://github.com/apache/superset/pull/44662#discussion_r4171705923
##########
superset/utils/cache.py:
##########
@@ -54,6 +54,67 @@ def generate_cache_key(values_dict: dict[str, Any],
key_prefix: str = "") -> str
return cache_key
+def exceeds_max_cache_value_size(cache_key: str, value: Any) -> bool:
+ """Check a value against ``DATA_CACHE_MAX_VALUE_SIZE`` before a data-cache
write.
+
+ This keeps one oversized value from flooding the cache backend (e.g.
+ Redis/Memcached) and evicting many smaller entries. When the serialized
+ (pickled) size of ``value`` is larger than the limit, a WARNING naming the
key
+ and size is logged and the ``skip_cache_value_too_large`` statsd counter is
+ incremented; the caller must then skip the write. Cache writers use
+ :func:`skip_oversized_cache_value`, which also removes any older value
stored
+ under the key.
+
+ :returns: ``True`` when the value is too large and must not be cached.
Always
+ ``False`` when ``DATA_CACHE_MAX_VALUE_SIZE`` is ``None``, in which
case the
+ value is not serialized and no overhead is incurred.
+ """
+ max_value_size = app.config.get("DATA_CACHE_MAX_VALUE_SIZE")
+ if max_value_size is None:
+ return False
+ value_size = len(pickle.dumps(value, protocol=pickle.HIGHEST_PROTOCOL))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Double serialization on cache write</b></div>
<div id="fix">
With the new default `DATA_CACHE_MAX_VALUE_SIZE = 10 * 1024 * 1024`
(config.py:1466), `exceeds_max_cache_value_size` now pickles every value
(cache.py:75) and the caller pickles it AGAIN in `cache.set` (executor.py:1173,
datasource/api.py:76, cache.py:165). Previously the `None` default skipped the
measurement pickle entirely. Every data-cache write now serializes twice on the
hot path — a real regression for large chart/SQL results. Consider reusing the
measured pickled bytes for the write, or a cheap size pre-check.
</div>
</div>
<small><i>Code Review Run #c55fc3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -3614,11 +3626,133 @@ def test_truncation_survives_cache_round_trip(
)
],
)
- cache_set = mocker.patch.object(cache_manager.data_cache, "set")
+ cache_set = enabled_data_cache.set
executor._store_in_cache(result, "SELECT 1", QueryOptions())
- mocker.patch.object(
- cache_manager.data_cache, "get",
return_value=cache_set.call_args.args[1]
- )
+ enabled_data_cache.get.return_value = cache_set.call_args.args[1]
cached = executor._get_from_cache("SELECT 1", QueryOptions())
assert cached is not None
assert cached.statements[0].truncated is truncated
+
+
+def _store_in_cache_with_cap(
+ mocker: MockerFixture,
+ database: Database,
+ data_cache: MagicMock,
+ row_count: int,
+) -> tuple[MagicMock, MagicMock, MagicMock]:
+ """Run ``_store_in_cache`` for a ``row_count``-row result under a 1 KB cap.
+
+ :returns: the mocked ``data_cache.set``, stats logger, and cache-module
logger
+ """
+ from superset_core.queries.types import (
+ QueryResult as QueryResultType,
+ StatementResult,
+ )
+
+ from superset.sql.execution.executor import SQLExecutor
+
+ stats_logger = MagicMock()
+ mocker.patch.dict(
+ current_app.config,
+ {
+ "DATA_CACHE_MAX_VALUE_SIZE": 1024,
+ "STATS_LOGGER": stats_logger,
+ "CACHE_DEFAULT_TIMEOUT": 300,
+ },
+ )
+ mock_cache_set = data_cache.set
+ mock_logger = mocker.patch("superset.utils.cache.logger")
+
+ result = QueryResultType(
+ status=QueryStatus.SUCCESS,
+ statements=[
+ StatementResult(
+ original_sql="SELECT name FROM users",
+ executed_sql="SELECT name FROM users",
+ data=pd.DataFrame(
+ {"name": [f"user-{i:05d}" for i in range(row_count)]}
+ ),
+ row_count=row_count,
+ )
+ ],
+ )
+ SQLExecutor(database)._store_in_cache(
+ result, "SELECT name FROM users", QueryOptions()
+ )
+ return mock_cache_set, stats_logger, mock_logger
+
+
+def test_store_in_cache_skips_oversized_result(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ enabled_data_cache: MagicMock,
+) -> None:
+ """A result larger than ``DATA_CACHE_MAX_VALUE_SIZE`` is not written; the
skip
+ is logged and counted. A later ``_get_from_cache`` simply misses and the
query
+ re-runs."""
+ mock_cache_set, stats_logger, mock_logger = _store_in_cache_with_cap(
+ mocker, database, enabled_data_cache, row_count=500
+ )
+
+ mock_cache_set.assert_not_called()
+ # An older result under the same key is removed so it is not served later.
+ enabled_data_cache.delete.assert_called_once()
+ stats_logger.incr.assert_called_once_with("skip_cache_value_too_large")
+ mock_logger.warning.assert_called_once()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing serialization assertion</b></div>
<div id="fix">
`test_store_in_cache_skips_oversized_result` asserts
`cache.set`/`delete`/stats but never that the value was serialized. The skip
decision in `exceeds_max_cache_value_size` (`superset/utils/cache.py:75`)
depends on `pickle.dumps`; the sibling NullCache test (3741, 3756) asserts
this. Without it, a regression that skips serialization entirely would still
pass this test. ([BITO.md 9460])
</div>
</div>
<small><i>Code Review Run #c55fc3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
