aminghadersohi commented on code in PR #44262:
URL: https://github.com/apache/superset/pull/44262#discussion_r4035028005
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
[email protected](
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect:
Any) -> None:
+ """Limit history to candidate entities only when a batch scope is
supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now,
scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
[email protected](
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
[email protected]("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first
connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
[email protected](
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
+ }
+ for id_, entity_type, day in zip(
+ ids, ["chart", "dashboard", "chart"], [1, 2, 3], strict=True
+ )
+ ]
+ try:
+ metadata.create_all(engine)
+ with engine.begin() as connection:
+ connection.execute(sa.insert(table), rows)
+ predicate: Callable[..., sa.ColumnElement[bool]]
+ for predicate in (
+ prune_audit._window_repeats_an_earlier_block,
+ prune_audit._legacy_repeats_an_earlier_block,
+ ):
+ assert set(
+ connection.scalars(
+ sa.select(table.c.id).where(
+ table.c.id.in_(ids if scope_entities != [] else
[]),
+ predicate(table, datetime(2026, 2, 1)),
Review Comment:
`scope_entities` is never passed to `predicate(...)`, so `None` and the
two-pair case run identically and `[]` only asserts an empty id list. The
parameter reads as scoped/unscoped coverage it does not provide here.
##########
tests/unit_tests/commands/deletion_retention/test_prune_audit.py:
##########
@@ -429,3 +433,621 @@ def test_pruning_shares_the_audit_writers_clock() -> None:
from superset.commands.deletion_retention import audit
assert prune_audit.utc_now is audit.utc_now
+
+
[email protected](
+ "dialect", [postgresql.dialect(), mysql.dialect(), sqlite.dialect()]
+)
+def test_repeat_history_scope_is_only_present_for_locked_rechecks(dialect:
Any) -> None:
+ """Limit history to candidate entities only when a batch scope is
supplied."""
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ now: datetime = datetime(2026, 1, 1)
+ scope_entities: list[tuple[str, str | None]] = [
+ ("dashboard", "b"),
+ ("chart", "a"),
+ ("chart", "a"),
+ ]
+ scoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now,
scope_entities=scope_entities)
+ )
+ unscoped: sa.sql.Select = sa.select(table.c.id).where(
+ *prune_audit._duplicate_predicates(table, now)
+ )
+ scoped_sql: str = str(
+ scoped.compile(dialect=dialect, compile_kwargs={"literal_binds": True})
+ )
+ unscoped_sql: str = str(unscoped.compile(dialect=dialect))
+ assert scoped_sql.count("entity_type IN ('chart', 'dashboard')") == 3
+ assert scoped_sql.count("entity_uuid IN ('a', 'b')") == 3
+ assert "repeat_scope" not in scoped_sql
+ assert "entity_type IN" not in unscoped_sql
+ assert "entity_uuid IN" not in unscoped_sql
+
+
+def test_maximum_literal_scope_executes_with_sqlite_bind_budget() -> None:
+ """Account for repeated positional scope binds at the 100-id ceiling."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(prune_audit.MAX_BATCH_SIZE)]
+ pairs: list[tuple[str, str | None]] = [
+ (f"type-{i}", f"entity-{i}") for i in range(len(ids))
+ ]
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ table.c.id.in_(ids),
+ *prune_audit._duplicate_predicates(
+ table, datetime(2026, 1, 1), scope_entities=pairs
+ ),
+ )
+ compiled: sa.sql.compiler.Compiled = query.compile(
+ dialect=sqlite.dialect(), compile_kwargs={"render_postcompile": True}
+ )
+ assert 300 < len(compiled.params) < 350
+ assert compiled.positiontup is not None
+ assert 700 < len(compiled.positiontup) < 750
+ try:
+ metadata.create_all(engine)
+ with engine.connect() as connection:
+ assert list(connection.scalars(query)) == []
+ finally:
+ engine.dispose()
+
+
[email protected](
+ ("backend", "version", "uses_window"),
+ [
+ ("mysql", (5, 7, 44), False),
+ ("mysql", (8, 0, 36), True),
+ ("mysql", (8, 4, 0), True),
+ ("postgresql", (17, 0), True),
+ ("sqlite", (3, 45, 0), True),
+ ],
+)
[email protected]("initialized", [True, False])
+def test_repeat_dispatch_uses_metadata_server_version(
+ backend: str,
+ version: tuple[int, ...],
+ uses_window: bool,
+ initialized: bool,
+) -> None:
+ """Route compatible MySQL servers by version, including first
connection."""
+ dialect: sa.engine.Dialect = mysql.dialect()
+ dialect.name = backend
+ dialect.server_version_info = version if initialized else None
+ connected_dialect: sa.engine.Dialect = mysql.dialect()
+ connected_dialect.name = backend
+ connected_dialect.server_version_info = version
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__
+ mock_db: MagicMock
+ with patch.object(prune_audit, "db") as mock_db:
+ mock_db.session.get_bind.return_value.dialect = dialect
+ mock_db.session.connection.return_value.dialect = connected_dialect
+ query: sa.sql.Select = sa.select(table.c.id).where(
+ prune_audit._repeats_an_earlier_block(table, datetime(2026, 2, 1))
+ )
+ sql: str = str(query.compile(dialect=mysql.dialect())).lower()
+ assert ("lag(" in sql) is uses_window
+ assert mock_db.session.connection.call_count == (
+ 1 if backend == "mysql" and not initialized else 0
+ )
+
+
[email protected](
+ "scope_entities",
+ [None, [("chart", "shared"), ("dashboard", "shared")], []],
+)
+def test_repeat_paths_isolate_entity_types_and_agree(
+ scope_entities: list[tuple[str, str | None]] | None,
+) -> None:
+ """Keep each type's first block when UUIDs collide, in both query paths."""
+ engine: sa.Engine = sa.create_engine("sqlite://")
+ metadata: sa.MetaData = sa.MetaData()
+ table: sa.Table = prune_audit.PurgeAuditLog.__table__.to_metadata(metadata)
+ ids: list[UUID] = [uuid4() for _ in range(3)]
+ rows: list[dict[str, Any]] = [
+ {
+ "id": id_,
+ "entity_type": entity_type,
+ "entity_uuid": "shared",
+ "status": STATUS_BLOCKED,
+ "trigger": "scheduled",
+ "actor": "system",
+ "created_on": datetime(2026, 1, day),
+ "reason": "same",
Review Comment:
Follow-on to the LAG-partition thread: the legacy path's twin conjunct
`between.entity_type == table.entity_type` can be dropped with all 147 unit
tests and the 40-seed equivalence test green — every row here shares one
reason, so it is trivially satisfied. Varying reason by type kills it.
```suggestion
"reason": "same" if entity_type == "chart" else "other",
```
--
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]
