codeant-ai-for-open-source[bot] commented on code in PR #42598:
URL: https://github.com/apache/superset/pull/42598#discussion_r3679118187
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -229,6 +229,104 @@ 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_config(
+ {
+ "DB_SQLA_URI_VALIDATOR": None,
+ "SUPERSET_META_DB_LIMIT": 2,
+ "DATABASE_OAUTH2_CLIENTS": {},
+ "SQLALCHEMY_CUSTOM_PASSWORD_STORE": None,
+ }
+)
+@with_feature_flags(ENABLE_SUPERSET_META_DB=True)
Review Comment:
**Suggestion:** This test uses `with_config`, whose wrapper restores
configuration only after the wrapped function returns. If this test fails or
raises during engine creation or query execution, `SUPERSET_META_DB_LIMIT`
remains set to `2` in the shared Flask application and can alter subsequent
tests, causing order-dependent failures. The configuration helper needs
exception-safe restoration, or this test needs equivalent cleanup. [stale
reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Failed tests can contaminate subsequent test configuration.
- ⚠️ Later metadata queries may unexpectedly limit rows.
- ⚠️ Test outcomes can become order-dependent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b82dcf3b8e0d4105b4cf872ed7697947&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=b82dcf3b8e0d4105b4cf872ed7697947&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:** 276:284
**Comment:**
*Stale Reference: This test uses `with_config`, whose wrapper restores
configuration only after the wrapped function returns. If this test fails or
raises during engine creation or query execution, `SUPERSET_META_DB_LIMIT`
remains set to `2` in the shared Flask application and can alter subsequent
tests, causing order-dependent failures. The configuration helper needs
exception-safe restoration, or this test needs equivalent cleanup.
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=7900e3bb631f69526cd7b48657228492452ff9ac706060b80dd2fb732f7d4270&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=7900e3bb631f69526cd7b48657228492452ff9ac706060b80dd2fb732f7d4270&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -229,6 +229,104 @@ 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_config(
+ {
+ "DB_SQLA_URI_VALIDATOR": None,
+ "SUPERSET_META_DB_LIMIT": 2,
+ "DATABASE_OAUTH2_CLIENTS": {},
+ "SQLALCHEMY_CUSTOM_PASSWORD_STORE": None,
+ }
+)
+@with_feature_flags(ENABLE_SUPERSET_META_DB=True)
+def test_superset_joins_with_limit_drops_matches(
+ mocker: MockerFixture,
+ 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.
+ """
+ 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}")
+
+ with engine.connect() as conn:
+ results = conn.execute(
+ text("""
+ SELECT t1.b, t2.b
+ FROM "database1.table1_large" AS t1
+ JOIN "database2.table2_late_match" AS t2
+ ON t1.a = t2.a
+ """)
+ )
Review Comment:
**Suggestion:** The test applies `LIMIT 2` without an `ORDER BY`, so the
database is free to return any two rows. If row `a=3` is included instead of
`a=1` or `a=2`, the join can return the expected row even while the per-table
truncation bug remains, making this regression test nondeterministic and
potentially ineffective. Add an explicit ordering to make the truncated rows
deterministic. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Regression test can pass while truncation remains.
- ⚠️ Join coverage depends on backend row-order behavior.
- ⚠️ CI results may vary across database configurations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=49c46756c44b40e6897278ee2c51fc86&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=49c46756c44b40e6897278ee2c51fc86&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:** 315:322
**Comment:**
*Logic Error: The test applies `LIMIT 2` without an `ORDER BY`, so the
database is free to return any two rows. If row `a=3` is included instead of
`a=1` or `a=2`, the join can return the expected row even while the per-table
truncation bug remains, making this regression test nondeterministic and
potentially ineffective. Add an explicit ordering to make the truncated rows
deterministic.
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=96ca3befa82f860783995bb1e7125824f4ee4a9f78de3fdfdb3e124c97b40fb6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=96ca3befa82f860783995bb1e7125824f4ee4a9f78de3fdfdb3e124c97b40fb6&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/test_sqlalchemy.py:
##########
@@ -229,6 +229,104 @@ 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_config(
+ {
+ "DB_SQLA_URI_VALIDATOR": None,
+ "SUPERSET_META_DB_LIMIT": 2,
+ "DATABASE_OAUTH2_CLIENTS": {},
+ "SQLALCHEMY_CUSTOM_PASSWORD_STORE": None,
+ }
+)
+@with_feature_flags(ENABLE_SUPERSET_META_DB=True)
+def test_superset_joins_with_limit_drops_matches(
+ mocker: MockerFixture,
+ 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.
+ """
+ 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 handler skips the test for every exception raised while
creating the engine, not just a missing dialect. Configuration errors, broken
dependencies, and other genuine regressions will therefore be reported as a
skipped test instead of failing CI, masking whether this regression is covered.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Engine regressions can be reported as skipped.
- ⚠️ The new join regression may receive no coverage.
- ⚠️ CI can remain green despite broken initialization.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=44e749eb77514219a308d2f867625a98&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=44e749eb77514219a308d2f867625a98&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:** 309:312
**Comment:**
*Possible Bug: The handler skips the test for every exception raised
while creating the engine, not just a missing dialect. Configuration errors,
broken dependencies, and other genuine regressions will therefore be reported
as a skipped test instead of failing CI, masking whether this regression is
covered.
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=95004197462ef87775fabdf43113dee2f6ca4352ae5fcbff7dea156409cc6169&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42598&comment_hash=95004197462ef87775fabdf43113dee2f6ca4352ae5fcbff7dea156409cc6169&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]
