codeant-ai-for-open-source[bot] commented on code in PR #42598:
URL: https://github.com/apache/superset/pull/42598#discussion_r3680964570
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -229,6 +230,108 @@ def test_superset_joins(
assert list(results) == [(10, "ten"), (20, "twenty")]
[email protected]
+def table1_large(session: Session, database1: "Database") -> Iterator[None]:
+ with database1.get_sqla_engine() as engine:
+ with engine.begin() as conn:
+ conn.execute(
+ text(
+ "CREATE TABLE table1_large (a INTEGER NOT NULL PRIMARY
KEY, "
+ "b INTEGER)"
+ )
+ )
+ conn.execute(
+ text("INSERT INTO table1_large (a, b) VALUES (1, 10), (2, 20),
(3, 30)")
+ )
+ db.session.commit()
+
+ yield
+
+ with engine.begin() as conn:
+ conn.execute(text("DROP TABLE table1_large"))
+ db.session.commit()
+
+
[email protected]
+def table2_late_match(session: Session, database2: "Database") ->
Iterator[None]:
+ with database2.get_sqla_engine() as engine:
+ with engine.begin() as conn:
+ conn.execute(
+ text(
+ "CREATE TABLE table2_late_match (a INTEGER NOT NULL
PRIMARY KEY, "
+ "b TEXT)"
+ )
+ )
+ conn.execute(
+ text("INSERT INTO table2_late_match (a, b) VALUES (3,
'thirty')")
+ )
+ db.session.commit()
+
+ yield
+
+ with engine.begin() as conn:
+ conn.execute(text("DROP TABLE table2_late_match"))
+ db.session.commit()
+
+
+@with_feature_flags(ENABLE_SUPERSET_META_DB=True)
+def test_superset_joins_with_limit_drops_matches(
+ mocker: MockerFixture,
+ monkeypatch: pytest.MonkeyPatch,
+ app_context: None,
+ table1_large: None,
+ table2_late_match: None,
+) -> None:
+ """
+ Regression for #36304: SUPERSET_META_DB_LIMIT is applied to each
+ underlying table independently, before the in-memory join runs. A row
+ that has a genuine match on the other side of the join but falls past
+ the per-table limit is silently dropped from the join result, with no
+ error or truncation warning.
+ """
+ # Use monkeypatch (rather than the `@with_config` decorator) so the
+ # config overrides are guaranteed to be undone even though this test is
+ # expected to fail its assertion until the underlying bug is fixed.
+ # `@with_config` only restores the original values after the wrapped
+ # test function returns normally, so an assertion failure here would
+ # otherwise leak SUPERSET_META_DB_LIMIT=2 into later tests.
+ monkeypatch.setitem(current_app.config, "DB_SQLA_URI_VALIDATOR", None)
+ monkeypatch.setitem(current_app.config, "SUPERSET_META_DB_LIMIT", 2)
+ monkeypatch.setitem(current_app.config, "DATABASE_OAUTH2_CLIENTS", {})
+ monkeypatch.setitem(current_app.config,
"SQLALCHEMY_CUSTOM_PASSWORD_STORE", None)
+
+ mocker.patch(
+ "superset.extensions.metadb.security_manager.raise_for_access",
+ return_value=None,
+ )
+
+ from flask import g
+
+ g.user = mocker.MagicMock()
+ g.user.is_anonymous = False
+
+ try:
+ engine = create_engine("superset://", future=True)
+ except Exception as e:
+ # Skip test if superset:// dialect can't be loaded (common in Docker)
+ pytest.skip(f"Superset dialect not available: {e}")
Review Comment:
**Suggestion:** The broad `Exception` handler skips the test for every
engine-construction failure, including genuine regressions in dialect
registration, feature-flag setup, or configuration. This can turn a broken
implementation into a silently skipped test instead of a CI failure; restrict
the skip to the specific missing-dialect/import error or allow unexpected
exceptions to propagate. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ CI can silently skip the regression test.
- ⚠️ Dialect initialization regressions remain undetected.
- ⚠️ Coverage for issue #36304 becomes environment-dependent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e6c4bee3e61f4897ac5efd8ea1814e8d&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=e6c4bee3e61f4897ac5efd8ea1814e8d&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/extensions/test_sqlalchemy.py
**Line:** 313:317
**Comment:**
*Possible Bug: The broad `Exception` handler skips the test for every
engine-construction failure, including genuine regressions in dialect
registration, feature-flag setup, or configuration. This can turn a broken
implementation into a silently skipped test instead of a CI failure; restrict
the skip to the specific missing-dialect/import error or allow unexpected
exceptions to propagate.
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%2F42598&comment_hash=a113616322b4da00245de2c0eb28388cc72eacc321b7e590e82c919ea94b8df6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=a113616322b4da00245de2c0eb28388cc72eacc321b7e590e82c919ea94b8df6&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]
