rusackas commented on code in PR #42598:
URL: https://github.com/apache/superset/pull/42598#discussion_r3694001831
##########
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:
Already fixed on the branch, swapped `@with_config` for `monkeypatch` so a
failed assertion here still restores `SUPERSET_META_DB_LIMIT` for later tests
instead of leaking it.
##########
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:
That broad except is the same skip pattern every other test in this file
uses (test_superset, test_superset_limit, test_dml, etc), just reused for the
new one. Not going to special-case it here, would rather keep it consistent.
##########
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:
Same as the other thread on this line, that's copied from the
skip-if-dialect-unavailable pattern already used by every test in the file.
Leaving it as-is for consistency rather than fixing it in just the new test.
##########
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:
With the actual adapter fix landed, join queries skip the per-table limit
entirely now, so there's no truncation happening here for row order to matter.
Should be moot at this point.
--
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]
