codeant-ai-for-open-source[bot] commented on code in PR #42464:
URL: https://github.com/apache/superset/pull/42464#discussion_r3664709337
##########
tests/unit_tests/datasets/test_datetime_format_detector.py:
##########
@@ -49,6 +50,23 @@ def mock_dataset() -> MagicMock:
lambda sql, limit, force: f"{sql} LIMIT {limit}"
)
+ # Mirror the real retry semantics: run the SQL unchanged first, and only
+ # on a read-limit rejection retry once with the ClickHouse-shaped
+ # bounded-read suffix.
+ def is_read_limit_error(ex: Exception) -> bool:
+ message = str(ex)
+ return "TOO_MANY_ROWS" in message or "Code: 158" in message
+
+ def run_with_retry(sql: str, run: Any) -> Any:
+ try:
+ return run(sql)
+ except Exception as ex:
+ if not is_read_limit_error(ex):
+ raise
+ return run(f"{sql}\nSETTINGS read_overflow_mode='break'")
Review Comment:
**Suggestion:** The test double does not mirror the real retry contract: if
the bounded-read retry fails, the production method restores and raises the
original read-limit exception, but this helper propagates the retry exception
instead. As a result, these tests cannot detect regressions where callers
receive the retry failure (for example, a readonly/settings error) instead of
the original `TOO_MANY_ROWS` error; preserve the first exception when the retry
raises. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Detector tests miss readonly retry-regression behavior.
- ⚠️ Failure assertions may validate the wrong exception.
- ❌ Production behavior is unaffected directly; this is test coverage loss.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `DatetimeFormatDetector.detect_column_format()` at
`superset/datasets/datetime_format_detector.py:94-96` invokes the
fixture-provided
`mock_dataset.database.run_with_sampling_read_limit_retry` with the
generated sampling SQL
and a callback that calls `database.get_df()`.
2. The fixture helper at
`tests/unit_tests/datasets/test_datetime_format_detector.py:60-66` runs the
original SQL
first and correctly identifies a read-limit exception before attempting the
bounded-read
retry.
3. Configure the callback to raise a ClickHouse read-limit exception for the
original SQL
and a different exception, such as readonly `Code: 164`, for the SQL
containing `SETTINGS
read_overflow_mode='break'`.
4. The fixture helper at
`tests/unit_tests/datasets/test_datetime_format_detector.py:66`
propagates the retry exception, whereas production
`Database.run_with_sampling_read_limit_retry()` at
`superset/models/core.py:356-359`
raises the original exception using `raise ex from retry_ex`. Consequently,
these detector
tests cannot catch a regression that surfaces the retry failure instead of
the original
`TOO_MANY_ROWS` error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c80d3f3d34b34f03920677f9d45e2dfb&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=c80d3f3d34b34f03920677f9d45e2dfb&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/datasets/test_datetime_format_detector.py
**Line:** 60:66
**Comment:**
*Api Mismatch: The test double does not mirror the real retry contract:
if the bounded-read retry fails, the production method restores and raises the
original read-limit exception, but this helper propagates the retry exception
instead. As a result, these tests cannot detect regressions where callers
receive the retry failure (for example, a readonly/settings error) instead of
the original `TOO_MANY_ROWS` error; preserve the first exception when the retry
raises.
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%2F42464&comment_hash=285e16d019a02e0ae12dc9871f370b6a2002d480f9940b77f6bb118c8ab9f906&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42464&comment_hash=285e16d019a02e0ae12dc9871f370b6a2002d480f9940b77f6bb118c8ab9f906&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]
