sadpandajoe commented on code in PR #45559:
URL: https://github.com/apache/superset/pull/45559#discussion_r4334769008
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -1844,31 +1883,68 @@ def evaluate_transform(
if not all(
literal_round_trips(value, dialect, database.backend) for value in
distinct
):
+ _count("declined.value_does_not_round_trip")
logger.warning(
"A filter value is not written the same way by the driver and the "
"database, so the partition transform is not probed at it; this "
"query will not prune"
)
return None
- cache_key = _probe_cache_key(database, catalog, schema, transform,
distinct)
- cached = _cache_get(cache_key)
+ cached = _cached_probe(database, catalog, schema, transform, distinct,
errors)
if cached is None:
- cached = _run_probe(
- database, catalog, schema, transform, distinct, errors=errors
- )
- if cached is None:
- # Deliberately not cached: a transient engine blip would otherwise
- # keep the dataset pruning-free for the whole cache timeout.
- return None
- _cache_set(cache_key, cached)
+ return None
evaluated = dict(
zip((_hashable(value) for value in distinct), cached, strict=False)
)
return [evaluated[_hashable(value)] for value in values]
+def _cached_probe(
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+ transform: str,
+ distinct: list[Any],
+ errors: list[str] | None,
+) -> list[Any] | None:
+ """
+ `_run_probe` behind the probe cache, remembering failures as well.
+
+ A failure is remembered only briefly. Not at all, and a transform the
Review Comment:
Now that failures are cached here, the `_run_probe` docstring (around line
1968) still says a failed probe is "deliberately not cached, so the next chart
re-probes and re-records". A reader trusting that will expect an immediate
retry after a transient error, when the verdict and probe both stay put for up
to `PARTITION_TRANSFORM_PROBE_FAILURE_CACHE_TIMEOUT`. Could that sentence be
updated alongside the other stale ones you already fixed in
`record_mirror_verdict`?
##########
tests/unit_tests/connectors/sqla/partition_mapping_test.py:
##########
@@ -2112,8 +2113,40 @@ def
test_the_cache_key_includes_the_catalog_and_schema(app: Flask) -> None:
assert _probe(database).call_count == 2
-def test_a_failed_probe_is_not_cached(app: Flask) -> None:
- """Caching a failure would keep a transient engine blip pruning-free for a
day."""
+def test_a_failed_probe_is_remembered_briefly(app: Flask) -> None:
+ """
+ A transform the engine always rejects must not cost a warehouse round trip
+ on every chart load -- and the preview must still be able to say why.
+ """
+ database = Database(database_name="probe_db", sqlalchemy_uri="sqlite://")
+ database.get_df = MagicMock( # type: ignore[method-assign]
+ side_effect=RuntimeError("function no_such_fn does not exist")
+ )
+ errors: list[str] = []
+
+ with app.app_context():
+ assert evaluate_transform(database, None, None, "lower(:value)",
["x"]) is None
+ assert (
+ evaluate_transform(
+ database, None, None, "lower(:value)", ["x"], errors=errors
+ )
+ is None
+ )
+
+ _probe(database).assert_called_once()
+ assert errors == ["function no_such_fn does not exist"]
+
+
+def test_a_failed_probe_is_retried_once_its_failure_expires(
+ app: Flask, mocker: MockerFixture
+) -> None:
+ """
+ Remembering a failure for the full cache timeout would keep a transient
+ engine blip pruning-free for a day; zero turns the failure cache off.
+ """
+ mocker.patch.dict(
Review Comment:
This test is named "retried once its failure expires", but a failure TTL of
`0` just disables the failure cache, so no entry ever exists to expire. If a
regression stored failures under the 24-hour success TTL (or ignored the new
setting), both failure-cache tests would still pass and a transient blip would
leave the dataset pruning-free for a day. Could you use a positive TTL and move
the cache clock (or expire the entry) between the two `evaluate_transform`
calls, asserting one `get_df` call before expiry and a second after?
--
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]
