shivamgoel commented on issue #37535:
URL: https://github.com/apache/superset/issues/37535#issuecomment-5417587818
## Revised proposal — supersedes the original above
The original predates the merged Semantic Layer work, and several
assumptions no longer hold. Reviewed with
@villebro. Draft implementation: #43527.
**Changed from the original:** the semantic-layer pull requests are no
longer a blocking dependency, they
merged; no command in `superset-core`, which contains no query execution; no
type-dispatch layer, since
`Explorable.get_query_result` already dispatches; built on
`QueryContextFactory` rather than
`QueryObjectFactory`; `SemanticQuery` supplies the vocabulary, not the wire
type; `dimensions` / `limit` /
`offset` / `order` replace the earlier field names; ad-hoc metrics are
explicitly in scope; `arrow` is a
per-request opt-in rather than deferred; and the separate capabilities
endpoint is dropped.
## Motivation
Querying a datasource by its semantic definitions today means `POST
/api/v1/chart/data`, which is built for
the Explore user interface. Callers hand-construct a `query_context` —
`BASE_AXIS` adhoc columns,
`post_processing` pivot and flatten chains, `extras`, `orderby` tuples —
which is frontend implementation
detail, not an API. Nothing validates names, so a misspelled metric surfaces
as a SQL error or an empty
result. And the payload follows Explore, so external consumers inherit every
change made for visualization
reasons. In practice they reverse-engineer a `query_context` from a saved
chart's `form_data`, which breaks
whenever the chart type changes.
The right abstraction already exists: the `Explorable` protocol
(`superset/explorables/base.py`), implemented
by both `SqlaTable` and the semantic-layer `SemanticView`, whose
`get_query_result` already routes to SQL or
to a semantic-layer provider. What's missing is a REST surface over it.
## Proposed Change
Two routes on the **existing** `DatasourceRestApi`, which already serves
this shape for `/compatible`,
`/column/<col>/values/` and `/validate_expression`, so no new API class or
permission view menu is needed:
```
POST /api/v1/datasource/<type>/<id>/query
GET /api/v1/datasource/<type>/<id> (metadata + capabilities)
```
`<type>` is a `DatasourceType` — `table` or `semantic_view`.
```
request
v
DatasourceQuerySchema.load() invalid type -> 400
| semantic_view + flag off -> 404
v
resolve_explorable() datasource lookup + raise_for_access()
v
validate_query_names() unknown name -> 400 + suggestion
v
build_query_dict() request vocabulary -> QueryObject
vocabulary
v
execute_tabular_query() QueryContextFactory.create()
| set_query_context_form_data()
| ChartDataCommand.validate() <-
authorization gate
| ChartDataCommand.execute()
v
Explorable.get_query_result() SqlaTable -> SQL
SemanticView -> mapper -> provider
```
Entering the pipeline at `QueryContextFactory` is the central design choice:
result caching, row-level
security, post-processing, time offsets, event logging and row-limit
clamping are all applied at or below that
boundary, so they come along unchanged. Nothing in the query engine is
modified.
### The shared core
Resolve, validate, build and execute live in
`superset/common/tabular_query.py`. `resolve_explorable` reads
`Explorable.metrics` and `.columns`, so both datasource types take **one**
code path.
The REST layer owns this core as the stable versioned contract; other
surfaces consume it. That matters now,
because two Model Context Protocol tools each implement the sequence inline
and have already drifted:
| | `get_table` | `query_dataset` |
| --- | --- | --- |
| `set_query_context_form_data` (Jinja macros) | no | yes |
| `custom_cache_timeout` honoured | no | yes |
| dimension field name | `dimensions` | `columns` |
| `extras.time_grain_sqla` | no | no |
Row one is a live bug: the same query against a Jinja-templated virtual
dataset returns **different results**
through the two tools. Both are refactored onto the shared core, and their
existing unit tests pass
**unmodified** — the evidence the refactor preserves behaviour.
## New or Changed Public Interfaces
```json
{
"metrics": ["sum__sales", {"expressionType": "SQL",
"sqlExpression": "SUM(a)/SUM(b)", "label":
"Ratio"}],
"dimensions": ["region", "product_category"],
"filters": [{"col": "region", "op": "IN", "val": ["EMEA", "APAC"]}],
"time_range": "Last 30 days",
"time_column": "order_date",
"time_grain": "P1D",
"limit": 1000,
"offset": 0,
"order": [{"column": "sum__sales", "descending": true}],
"result_format": "json"
}
```
| Field | Notes |
| --- | --- |
| `metrics`, `dimensions` | `List(Raw)`, because `Metric` is `AdhocMetric \|
str` — identical to `ChartDataQueryObjectSchema`, so ad-hoc expressions work on
datasets at no cost. Semantic views reject them by design, since the provider
owns metric definitions; surfaced as 400 naming the metric, never a 500 |
| `filters` | Reuses `ChartDataFilterSchema`, which already validates
against `FilterOperator` |
| `time_column`, `time_grain` | A column plus an ISO 8601 duration. The
grain is applied through a `BASE_AXIS` adhoc column, because
`extras.time_grain_sqla` is read only by the semantic-layer mapper and
**ignored for datasets** — setting just that would silently return unbucketed
rows. No resolvable temporal column returns 400 |
| `limit`, `offset`, `order` | `SemanticQuery`'s names rather than
`QueryObject`'s, with per-column sort direction mirroring its `OrderTuple`.
Translation happens once, in `build_query_dict`, keeping the contract
independent of the execution model |
| `result_format` | `json` (default) or `arrow` |
JSON returns the conventional `{"result": [{data, colnames, coltypes,
rowcount, is_cached, ...}]}` envelope.
`arrow` returns an Apache Arrow stream as
`application/vnd.apache.arrow.stream`. Arrow needs no feature flag,
since it is selected per request; no cache change, since serialization
happens at response time and both
formats share cache entries; and is deliberately excluded from
`table_like()`, so it is not gated behind
export permissions — it is a programmatic transport, not a file export.
`GET /api/v1/datasource/<type>/<id>` returns `datasource.data` plus a
`capabilities` block built from
properties that already exist on `Explorable`: `is_rls_supported`,
`query_language`, `supports_samples`,
`supports_drill_to_detail`, `supports_adhoc_metrics`, `time_grains`,
`supported_operators`, and `features`.
Compatible metric and dimension discovery is not duplicated — `POST
…/compatible` already provides it with a
security-aware cache.
### Operators
Three sets exist and should not be conflated: the wire set
(`FilterOperator`, whose values are `==` rather
than `EQUALS`, and which includes `TEMPORAL_RANGE`), the driver set
(`Operator` in `superset-core`), and what
the mapper accepts. `TEMPORAL_RANGE` never reaches a driver — the mapper
expands it into `>=` and `<` bounds.
The endpoint accepts the full wire set and returns 400 naming the operator
when a datasource cannot express it.
`ILIKE` and `NOT_ILIKE` previously **raised** for semantic views, with a
comment arguing that collapsing
case-sensitivity onto the backend's collation diverges from the operator the
author chose, and a test
asserting the rejection. This proposal implements them instead; if a
provider cannot express
case-insensitive matching, the fallback is a `SemanticViewFeature` gate,
which already exists for this.
### Security, permissions and feature flag
`@protect()` and `raise_for_access()` guard the route, but
`ChartDataCommand.validate()` is the real gate —
it applies query-context authorization and row-level security, so no second
authorization path is written.
Errors are sanitized for low-privilege callers. Guest tokens are out of
scope. Per `SECURITY.md`, this grants
nothing beyond what datasource read access, `raise_for_access` and row-level
security already permit through
`/api/v1/chart/data`.
Both routes share one `can_query on Datasource` permission, reachable by the
Gamma role because `"can_query"`
is added to `READ_ONLY_PERMISSION` — the same treatment `can_get_drill_info`
already has. Without it,
`Datasource` being listed in `GAMMA_READ_ONLY_MODEL_VIEWS` makes
`_is_alpha_only` withhold the permission.
The metadata route is deliberately **not** named `get`, because
`PUBLIC_ROLE_PERMISSIONS` already grants
`("can_get", "Datasource")` for chart rendering, and a method named `get`
would expose datasource metadata to
unauthenticated users wherever `PUBLIC_ROLE_LIKE = "Public"`.
*Related pre-existing issue, out of scope:* `can_get_column_values` and
`can_compatible` on this class are
subject to the same exclusion and are unreachable by Gamma today.
Consolidating them is a breaking permission
change needing a migration.
`superset.datasource.api.query` is added to `WTF_CSRF_EXEMPT_LIST`, as
`/api/v1/chart/data` already is, since
a token-authenticated client has no cross-site request forgery token to
send; without it every non-browser
caller receives *"The CSRF token is missing"*. Stated plainly rather than
buried: with
`allow_browser_login = True` this means a browser session cookie alone can
drive the endpoint cross-site — the
same trade-off `/api/v1/chart/data` accepts, with the same mitigation, being
read-only and behind the
authorization gate above.
The route is always registered. `SEMANTIC_LAYERS` (default off) gates only
the semantic-view branch: `table`
always works, `semantic_view` returns 404 when off. Gating the whole route
would make the dataset case
unreachable in nearly every deployment.
## What this changes
Three new files — `superset/common/tabular_query.py` and two test modules —
and twelve modified:
| File | Change |
| --- | --- |
| `superset/datasource/api.py` | two new methods |
| `superset/datasource/schemas.py` | one new schema |
| `superset/mcp_service/**/{get_table,query_dataset}.py` | call the shared
core |
| `superset/security/manager.py` | `can_query` in `READ_ONLY_PERMISSION` |
| `superset/config.py` | query route exempted from cross-site request
forgery checks |
| `superset-core/.../semantic_layers/types.py` | `ILIKE` / `NOT_ILIKE` on
`Operator` |
| `superset/semantic_layers/mapper.py` | implement those instead of raising |
| `superset/common/chart_data.py` | `ARROW` on `ChartDataResultFormat` |
| `superset/common/query_context_processor.py` | Arrow serializer branch |
| `superset/explorables/base.py` | declare `raise_for_access` on the
protocol |
| `tests/unit_tests/semantic_layers/mapper_test.py` | replace the rejection
test |
The `explorables/base.py` change is worth noting: every explorable
implements `raise_for_access` and callers
depend on it, but the protocol never declared it, so type checking rejects
the call. Safe at runtime, since
nothing performs an `isinstance` check against the protocol.
**Untouched:** `QueryObject`, `QueryContext`, `QueryContextFactory`,
`QueryObjectFactory`, `ChartDataCommand`,
`DatasourceType`, the frontend, and `/api/v1/chart/data`.
**None of:** database migrations, new feature flags, new API classes, new
view menus, new dependencies.
`pyarrow` is already a direct dependency, so Arrow adds nothing.
## Migration Plan and Compatibility
Purely additive at the HTTP layer, no database migrations. One configuration
default changes
(`WTF_CSRF_EXEMPT_LIST`), requiring no deployment action.
`/api/v1/chart/data` is unchanged and remains the
endpoint for chart-shaped queries, `post_processing`, `annotation_layers`
and asynchronous execution.
Two internal changes disclosed rather than hidden: a name-validation helper
relocates out of the Model Context
Protocol package, with a one-release re-export at the old path; and the two
tools lose their inline
implementations. Their public schemas are unchanged, but `get_table`
**gains** Jinja and cache-timeout
correctness — a fix that changes results for anyone relying on the broken
behaviour, so it warrants an
`UPDATING.md` note. Deployments that grant permissions explicitly rather
than through role bundles need
`can_query on Datasource`.
## Rejected and deferred
**`SemanticQuery` as the wire format.** It holds `pyarrow` data types and
*resolved* `Dimension` and `Metric`
objects rather than names, so it is the output of resolution rather than a
serializable input; this proposal
takes its terminology, not its type.
**`SemanticQuery` as the single internal query representation — deferred.**
The core `SemanticView` interface
has no implementations in the repository, so datasets would need a
`SqlaTable`-backed driver, effectively a
second SQL generator; and caching, row-level security, post-processing and
clamping all sit above the driver,
so such a path would re-implement them or re-enter this pipeline.
`tabular_query.py` is the seam if it is
pursued later, and the payload would not change.
**Building on `QueryObjectFactory`.** It is invoked inside
`QueryContextFactory`; entering below that forfeits
everything listed under Proposed Change.
**A command in `superset-core`.** No query execution exists there; that
refactor belongs to SIP-187.
**A separate capabilities endpoint.** Every field already exists on
`Explorable` or is served by `/compatible`.
**Per-type endpoints and aliases** such as `/dataset/<id>/query`. One schema
with `<type>` in the path is
simpler, and dispatch already exists below the API layer.
**Group limits and asynchronous execution.** Both need extra wire fields and
capability negotiation; the
payload is forward-compatible with either.
## Verification
Covered by unit tests and manual verification against the examples dataset:
existing Model Context Protocol
tests pass unmodified; the Jinja divergence resolves across all three
surfaces; `can_query on Datasource`
exists after `superset init` and the Public role gains neither route;
`ILIKE` on a semantic view returns rows
rather than raising; an ad-hoc metric on a semantic view returns 400 naming
it; omitting `result_format`
returns JSON; `arrow` round-trips to the same rows as JSON and shares its
cache entry; `time_grain` emits a
real date truncation with a matching `GROUP BY`; `semantic_view` returns 404
with the flag off; `limit` above
the schema cap returns 400; a repeated query reports a cache hit and `force`
bypasses it.
**Two items are not yet covered in #43527** and need integration rather than
unit tests: that row-level
security applies to queries issued through this endpoint, and that a Gamma
user with dataset access can call
it while the Public role cannot.
--
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]
