codeant-ai-for-open-source[bot] commented on code in PR #31981:
URL: https://github.com/apache/superset/pull/31981#discussion_r3631582137
##########
superset/datasource/api.py:
##########
@@ -145,6 +200,31 @@ def get_column_values(
),
)
+ # Warn before caching very large payloads (high-cardinality columns)
+ # so operators can spot cache-memory pressure before Redis OOMs.
+ # Threshold is operator-tunable; defaults to 100k rows.
+ warn_threshold = app.config.get("FILTER_VALUES_CACHE_WARN_THRESHOLD",
100_000)
+ if (payload_size := len(payload)) > warn_threshold:
+ logger.warning(
+ "column-values payload exceeds cache-warn threshold: "
+ "uid=%s col=%s rows=%d threshold=%d",
+ datasource.uid,
+ column_name,
+ payload_size,
+ warn_threshold,
+ )
+
+ timeout = datasource.cache_timeout or app.config.get(
+ "CACHE_DEFAULT_TIMEOUT", 300
Review Comment:
**Suggestion:** Using boolean `or` to choose the timeout treats `0` as
falsy, so an explicit zero timeout is ignored and replaced by the default TTL,
causing entries to be cached when they should not be. Use an explicit `is not
None` check so `0` is preserved. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Zero cache_timeout cannot disable filter-value caching.
- ⚠️ Filter values cached longer than datasource configuration intends.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a datasource in Superset so that `BaseDatasource.cache_timeout`
is explicitly
set to `0` (via the dataset caching settings in the UI, persisted on the
model used by
`get_column_values()` in `superset/datasource/api.py`).
2. Open a dashboard that uses native filters backed by this datasource; the
frontend
issues a request to the filter-values endpoint, which reaches
`get_column_values()` and
executes the caching block at lines 155–227.
3. At lines 217–219, `timeout = datasource.cache_timeout or
app.config.get("CACHE_DEFAULT_TIMEOUT", 300)` runs; since
`datasource.cache_timeout == 0`
is falsy, `timeout` is set to the default from
`app.config["CACHE_DEFAULT_TIMEOUT"]`
instead of preserving the explicit `0`.
4. At lines 220–223, the code calls `cache_manager.data_cache.set(cache_key,
payload,
timeout=timeout)` and later requests return a cache HIT with header
`X-Cache-Status =
"HIT"`, showing that entries are cached for the default TTL even though the
datasource’s
cache timeout was set to zero.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c4d7f7f33c8b4297bdae2f8bd27a41f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c4d7f7f33c8b4297bdae2f8bd27a41f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/datasource/api.py
**Line:** 217:218
**Comment:**
*Logic Error: Using boolean `or` to choose the timeout treats `0` as
falsy, so an explicit zero timeout is ignored and replaced by the default TTL,
causing entries to be cached when they should not be. Use an explicit `is not
None` check so `0` is preserved.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F31981&comment_hash=746409104437fddecaa7923019606681f18f2797c3245aecd474acfaad4cde55&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F31981&comment_hash=746409104437fddecaa7923019606681f18f2797c3245aecd474acfaad4cde55&reaction=dislike'>👎</a>
##########
tests/unit_tests/result_set_test.py:
##########
@@ -450,3 +450,93 @@ def test_stringify_extension_columns() -> None:
# plain binary BLOBs and other types are left untouched
assert pa.types.is_binary(result.schema.field("blob").type)
assert pa.types.is_integer(result.schema.field("n").type)
+
+
+def test_stringify_values_dict_and_list_produce_valid_json() -> None:
Review Comment:
**Suggestion:** This test function name duplicates an existing test name
later in the same file, so one definition overwrites the other and silently
drops coverage. Rename one of the tests so both execute. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Newly added JSON behavior test never actually runs.
- ⚠️ Reduced regression coverage for stringify_values enhancements.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/result_set_test.py` and note the new definition
`def
test_stringify_values_dict_and_list_produce_valid_json() -> None:` added at
line 455, plus
an existing definition with the same name starting at line 545 in the
unchanged section.
2. Import this test module under pytest; Python executes definitions in
order, binding
`test_stringify_values_dict_and_list_produce_valid_json` first to the
function at lines
455–480 and then overwriting it with the later definition at lines 545+.
3. Run `pytest tests/unit_tests/result_set_test.py` and inspect the
collected tests; only
one test named `test_stringify_values_dict_and_list_produce_valid_json` is
present and
executed, corresponding to the last definition (the original test), while
the newly added
test body is never called.
4. As a result, the additional assertions in the new test (lines 476–487,
which verify
double-quoted JSON and JSON parsing behavior for dict/list values) do not
run, reducing
coverage of the new `stringify_values()` logic in
`superset/result_set.py:73–90` despite
the test code being present.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=111c2530408d4abaa194beb7ea371733&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=111c2530408d4abaa194beb7ea371733&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/result_set_test.py
**Line:** 455:455
**Comment:**
*Logic Error: This test function name duplicates an existing test name
later in the same file, so one definition overwrites the other and silently
drops coverage. Rename one of the tests so both execute.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F31981&comment_hash=19799fbe87cc2dbdfe036d40a0224a1accb87fa5b1e0b21bdeae185150cbc21f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F31981&comment_hash=19799fbe87cc2dbdfe036d40a0224a1accb87fa5b1e0b21bdeae185150cbc21f&reaction=dislike'>👎</a>
--
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]
