sadpandajoe commented on code in PR #43759:
URL: https://github.com/apache/superset/pull/43759#discussion_r4155597487
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -772,6 +772,87 @@ def is_transform_active(transform: str | None, engine:
str) -> bool:
return not validate_transform(transform, engine)
+def preview_partition_mapping(
+ datasource: SqlaTable,
+ *,
+ mapped_column: str,
+ value_transform: str | None,
+ sample_value: str,
+) -> dict[str, Any]:
+ """
+ Evaluate a candidate mapping and describe the predicate it would emit.
+
+ Shares the evaluator -- and therefore the probe cache -- with the query
+ path, so preview and runtime cannot drift and a previewed transform warms
+ the chart path for free.
+
+ Validation runs first and the engine second: a half-typed transform is by
+ definition unparseable, so most of what a text input produces costs zero
+ queries.
+ """
+ partition_column = datasource.partition_column
+ if not partition_column:
+ return {"valid": False, "error": _("No partition column is set.")}
+
+ column_names = {str(column.column_name) for column in datasource.columns}
+ if mapped_column not in column_names:
+ return {
+ "valid": False,
+ "error": _("%(name)s is not a column on this dataset.",
name=mapped_column),
+ }
+
+ engine = datasource.database.backend
+ for issue in validate_partition_mapping(
+ column_names=column_names,
+ partition_column=str(partition_column),
+ partition_mapped_column=mapped_column,
+ main_dttm_col=datasource.main_dttm_col,
+ transform=value_transform,
+ engine=engine,
+ ):
+ return {"valid": False, "error": str(issue.message)}
+
+ evaluated = evaluate_transform(
+ datasource.database,
+ datasource.catalog,
+ datasource.schema,
+ cast(str, value_transform),
+ [sample_value],
+ )
+ if evaluated is None:
+ return {
+ "valid": False,
+ "error": _("The transform could not be evaluated against the
database."),
+ }
+
+ return {
+ "valid": True,
+ "emitted_predicate": (
+ f"{datasource.quote_identifier(str(partition_column))} >= "
+ f"{_render_literal(datasource.database, evaluated[0])}"
+ ),
+ }
+
+
+def _render_literal(database: Database, value: Any) -> str:
+ """
+ Render a probed value the way it appears in the generated SQL.
+
+ Compiled by the dialect rather than formatted by hand. A probe returns
+ whatever the warehouse gave it -- an epoch integer, a date, a NULL for an
+ input the transform could not convert -- and only the dialect knows how
each
+ of those is written. `str()` would render a NULL as `None` and leave a date
+ unquoted, neither of which is SQL; hand-rolled quote-doubling would only
+ ever have been right for strings.
+ """
+ return str(
+ sa.literal(value).compile(
Review Comment:
A successful integer probe returns a NumPy scalar from the pandas row, but
`sa.literal()` cannot infer its SQL type, so the canonical epoch transform
raises `CompileError` here and preview returns 500. Could the probed scalar be
normalized or rendered with an explicit type, with coverage through a real
DataFrame result rather than a mocked Python `int`?
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -772,6 +772,87 @@ def is_transform_active(transform: str | None, engine:
str) -> bool:
return not validate_transform(transform, engine)
+def preview_partition_mapping(
+ datasource: SqlaTable,
+ *,
+ mapped_column: str,
+ value_transform: str | None,
+ sample_value: str,
+) -> dict[str, Any]:
+ """
+ Evaluate a candidate mapping and describe the predicate it would emit.
+
+ Shares the evaluator -- and therefore the probe cache -- with the query
+ path, so preview and runtime cannot drift and a previewed transform warms
+ the chart path for free.
+
+ Validation runs first and the engine second: a half-typed transform is by
+ definition unparseable, so most of what a text input produces costs zero
+ queries.
+ """
+ partition_column = datasource.partition_column
+ if not partition_column:
+ return {"valid": False, "error": _("No partition column is set.")}
+
+ column_names = {str(column.column_name) for column in datasource.columns}
+ if mapped_column not in column_names:
+ return {
+ "valid": False,
+ "error": _("%(name)s is not a column on this dataset.",
name=mapped_column),
+ }
+
+ engine = datasource.database.backend
+ for issue in validate_partition_mapping(
+ column_names=column_names,
+ partition_column=str(partition_column),
+ partition_mapped_column=mapped_column,
+ main_dttm_col=datasource.main_dttm_col,
+ transform=value_transform,
+ engine=engine,
+ ):
+ return {"valid": False, "error": str(issue.message)}
+
+ evaluated = evaluate_transform(
Review Comment:
A dataset editor without SQL Lab can submit a scalar subquery as the unsaved
transform and receive its warehouse result in `emitted_predicate`; this path
never applies the subquery/RLS checks used for stored expressions. Could the
shared SQL policy and row-level-security validation run before the probe, so
preview cannot read rows the editor is not authorized to retrieve?
##########
superset/datasets/api.py:
##########
@@ -158,6 +163,51 @@
)
+#: How long one preview budget lasts, in seconds.
+PREVIEW_RATE_LIMIT_WINDOW = 60
+
+
+def _consume_preview_rate_limit(dataset_id: int) -> bool:
+ """
+ Fixed-window per-user, per-dataset throttle on the preview endpoint.
+
+ Debouncing on the client is a courtesy, not a guard: a held keydown, or a
+ handful of owners with the editor open, becomes sustained load on a
+ production cluster. Returns False once the window's budget is spent.
+
+ Counting is `add` then `inc` rather than read-then-write, which buys two
+ things a `get`/`set` pair cannot. It is atomic where it matters -- on Redis
+ those are `SETNX` and `INCR`, so concurrent previews cannot each read the
+ same sub-limit value and all be let through. And the window is genuinely
+ fixed: only `add` sets a lifetime, so the budget expires a minute after the
+ *first* request rather than a minute after the most recent one, which is
+ what the name promises. (`INCR` leaves the TTL alone; a backend whose `inc`
+ is a read-modify-write may restore its own default lifetime instead, which
+ throttles for longer rather than shorter.)
+
+ Both calls go to the cachelib backend rather than the Flask-Caching wrapper
+ around it, which proxies `add` but not `inc`.
+ """
+ limit = app.config.get("PARTITION_TRANSFORM_PREVIEW_RATE_LIMIT", 30)
+ if not limit:
+ return True
+
+ user_id = get_user_id() or 0
+ key = f"partition_mapping_preview:{user_id}:{dataset_id}"
+ backend = cache_manager.cache.cache
+ try:
+ if backend.add(key, 1, timeout=PREVIEW_RATE_LIMIT_WINDOW):
+ # First request of a fresh window, and the only one that dates it.
+ return True
+ used = backend.inc(key)
Review Comment:
If the Redis key expires after `add()` returns false but before `inc()`,
`INCR` recreates it without a TTL. That counter then never resets, so this user
eventually gets permanent 429s for the dataset; could creation/increment/expiry
be atomic or missing expiry be repaired?
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -231,6 +233,10 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
is_active: column.is_active,
is_dttm: column.is_dttm,
python_date_format: column.python_date_format || null,
+ partition_value_transform: column.partition_value_transform || null,
Review Comment:
The Dataset List editor loads `/api/v1/dataset/{id}`, whose column response
omits both transform fields, so an unrelated save now sends `null`/`false` and
overwrites an existing mapping, silently disabling pruning. Could those fields
be included in the GET response and the GET→edit→PUT round trip be covered?
--
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]
