Repository navigation
feat(semantic-layers): add an SDK conformance kit for providers - #45395
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45395 +/- ##
==========================================
+ Coverage 84.57% 84.58% +0.01%
==========================================
Files 3069 3073 +4
Lines 205227 205734 +507
Branches 48222 48180 -42
==========================================
+ Hits 173561 174028 +467
- Misses 28362 28402 +40
Partials 3304 3304
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
requires:rebase
github-actions
Bot
added
github_actions
…rmance-kit # Conflicts: # docs/developer_docs/extensions/contribution-types.md # superset-core/src/superset_core/semantic_layers/testing.py # superset-core/tests/semantic_layers/example_provider.py # superset-core/tests/semantic_layers/test_registration.py
github-actions
Bot
removed
github_actions
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if case == "grain": | ||
| assert any(dimension.grain is not None for dimension in query.dimensions) |
There was a problem hiding this comment.
Suggestion: This accepts a single grained dimension and never checks another variant of the same name, so incorrect grain-variant selection can pass.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incomplete implementation
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-core/src/superset_core/semantic_layers/testing.py
**Line:** 123:124
**Comment:**
*Incomplete Implementation: This accepts a single grained dimension and never checks another variant of the same name, so incorrect grain-variant selection can pass.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Thanks. I don't think this is a real gap. The grain case doesn't stop at checking that the query has a grained dimension: the test then runs the query and compares the provider's result with the fixture's expected Arrow table, including column names and row order. If a provider picks the wrong grain variant, the bucketed rows differ and the comparison fails.
Requiring a second variant of the same dimension in the catalog would also wrongly fail providers that only expose one grain. Making the expected table actually tell grains apart is the fixture author's job, just as the existing num_rows > 0 guard covers the no-data case. Leaving this as is.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
For semantic-query grain tests, verify grain selection by comparing the query result with an expected fixture that distinguishes the grain; do not require every provider to expose multiple grain variants.
Applied to:
**/test/****/tests/****/*test*.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Verdict: no blocking issues; several non-blocking gaps where the kit is stricter or looser than the host contract.
Ran superset-core/tests on the PR head (48 passed; CI green). I also ran the example suite against provider mutations:
| Mutation | Result |
|---|---|
| truncate-then-sort | caught (order_limit) |
| fault reported as refusal | caught (3 fault cases) |
| stale catalog on unbound construction | caught |
get_values returns {x, y} columns and ignores filters |
passes |
get_row_count returns {"nonsense": ["x"]} |
passes |
fault path raises TypeError (provider bug) |
passes |
| result columns metrics-first | fails 7 cases |
null-free columns marked nullable=False |
fails 7 cases |
The last two show the kit is stricter than the host (inline). The get_values/get_row_count rows show it is looser. The doc's "count lane only checks failure propagation" disclaimer covers those two, but the host already relies on a fixed get_values shape. SemanticView.values_for_column in superset/semantic_layers/models.py reads the column named dimension.name (or a single column) and raises ValueError otherwise. A success case for that would catch a broken value-suggestion path that currently passes.
Other notes:
- Non-blocking. The documented opt-in (
pytest_pluginsplusfrom superset_core.semantic_layers.testing import SemanticProviderConformancein the same test module) imports the plugin before pytest can rewrite it. That emitsPytestAssertRewriteWarning: Module already imported so cannot be rewritten, and the bare asserts in the kit then fail with a plainAssertionErrorand no detail (for example, the fault lane shows nothing about which exception was raised). preset-io/superset-shell#5607 loads it with-p, so it gets rewriting, and itsknown_failuresregexes depend on the rewritten text. Either recommend-p superset_core.semantic_layers.testingor a rootconftest.pyin the doc, or callpytest.register_assert_rewrite/ give every assert an explicit message. - Nit. The doc says "Include at least two grains for one name", but the kit only checks
grain is not None(and the reply to the earlier bot comment argues one grain should be allowed). Align the doc with what is enforced. - Nit.
_check_isolationrejectsflask/flask_appbuilder, which are hard dependencies ofapache-superset-core. A provider importing an SDK module such assuperset_core.common.modelsfails with "Provider imported host module flask_appbuilder". This is probably intended, but the message blames the provider for an SDK import.
| assert full.results.num_rows > query.limit, "seed more rows than the limit" | ||
| assert full.results.slice(query.offset or 0, query.limit).equals(expected) | ||
| result: SemanticResult = semantic_view.get_table(query) | ||
| assert result.results.equals(expected), ( |
There was a problem hiding this comment.
Non-blocking (stricter than the contract). pa.Table.equals compares column order and field nullability. The SDK only promises "one column per selected member name" (types.py, view.py), and the host selects by name (cache_transform.py: results.select(columns)). A provider that returns metrics before dimensions, or marks null-free columns nullable=False, fails all 6 query cases and the capability case even though the host accepts it.
The nullability failure is also hard to diagnose because the message prints identical data:
AssertionError: identity: expected {'category': [None, 'A', 'B'], 'total': [3, 7, 8]}, got {'category': [None, 'A', 'B'], 'total': [3, 7, 8]}
Suggest comparing result.results.select(expected.column_names) after normalizing nullability (or comparing types and values per column), and including schema in the failure message. The same applies to line 207.
There was a problem hiding this comment.
Fixed in 0cb0130. The kit compares typed columns by name, so field order and nullability no longer reject equivalent results. This applies to the query, order-before-limit and capability checks. The regressions cover reordered and nonnullable columns; wrong types, values, row order and duplicate names still fail, with schema details in the diagnostic.
One follow-up hardening item: the negative comparison regressions should match the specific assertion diagnostic, so a future fixture change cannot make an unrelated assertion satisfy the test.
| assert isinstance(error.value, SemanticResultCompletenessError) | ||
| assert error.value.reason == kind | ||
| else: | ||
| assert not isinstance( |
There was a problem hiding this comment.
Non-blocking (vacuous). The fault lane passes on any exception that is not an SDK refusal or completeness error, including TypeError, AttributeError, KeyError or AssertionError from a provider bug or a broken fixture. With the example's fault path changed to raise TypeError("'NoneType' object is not subscriptable"), all fault cases pass. Consider letting the fixture declare the expected fault type, for example {"get_table:fault": (callable, ConnectionError)}, or at least rejecting TypeError, AttributeError, NameError and AssertionError here.
There was a problem hiding this comment.
Fixed in 0cb0130. The fault lane rejects TypeError, AttributeError, NameError, AssertionError and KeyError, in addition to refusal/completeness exceptions. Each programming-error variant has a regression. Backend-specific exception classes remain provider-owned, and the guide calls for provider lanes to assert their exact backend fault type too.
This implements the minimum guard you suggested; it is still a heuristic, since other programming errors can pass. A fixture-declared expected fault type is the stronger follow-up, alongside the deferred value/count success cases.
| assert query.offset > 0 | ||
| assert query.limit is not None | ||
| assert query.limit > 0 | ||
| elif name in {"GROUP_LIMIT", "GROUP_OTHERS"}: |
There was a problem hiding this comment.
Non-blocking (looser). Only ROW_OFFSET and GROUP_* witnesses are checked for actually using the feature. For ADHOC_COLUMN_EXPRESSIONS and ADHOC_EXPRESSIONS_IN_ORDERBY, any plain query satisfies the lane, so "every advertised feature has a result-level witness" does not hold for them. An AdhocExpression should be required in dimensions or order respectively.
Also, a single test loops over every feature, so the first failing feature hides the others. Parametrizing over SemanticViewFeature, with a skip when the feature is not advertised, would report each one separately.
There was a problem hiding this comment.
Fixed in 0cb0130. Each SDK capability is a separate parametrized case, with an explicit skip when unadvertised. The two ad-hoc cases require an AdhocExpression in the selected dimensions or order keys respectively; plain-query witnesses fail before provider execution.
| # A captured view must never claim a later discovery token. | ||
| assert first.metadata_cache_token in {None, "generation-a"} | ||
| semantic_change_catalog() | ||
| unbound: SemanticView = semantic_make_view(None) |
There was a problem hiding this comment.
Non-blocking (stricter than the contract). No host path constructs a provider without a token. SemanticLayer.from_configuration takes cache_token: str (required, not optional), and its only host caller (execution.py, the bind method) always passes layer.metadata_generation. The "unbound" obligation exists only in this kit and in the sentence this PR adds to semantic-layer-cache-clear.md.
As a result, adapters have to invent a tokenless path. preset-io/superset-shell#5607 already calls from_configuration(..., cache_token=None) against the str annotation. Either drop the None leg (keep the two-generation freshness and no-relabel checks), or make the SDK signature str | None and document when the host uses it.
There was a problem hiding this comment.
Agreed; fixed in 0cb0130 by dropping the unsupported None-construction leg and its cache-guide sentence. The kit passes string tokens matching from_configuration and the host binding path. It still verifies catalog freshness across two generations and that an older captured view is never relabeled. This does not change the SDK factory signature or legacy metadata_cache_token=None echoes. The shell adapter should use that required string-token path rather than widen the SDK contract.
| ) | ||
| assert completed.returncode == 0, completed.stdout + completed.stderr | ||
|
|
||
| # TODO: add the default chronological-order case after its SDK rule lands. |
There was a problem hiding this comment.
Nit. This TODO is stale. #45389 landed the rule, and it is host-side: with_default_order fills order before dispatch, and the SemanticQuery docstring says a get_table call "receives an empty order only when no temporal dimension is selected". The provider-side obligation is just "honour the explicit order", which order_limit/grain already cover. The same applies to the "Default chronological ordering will be added after that SDK rule lands" sentence in semantic-layer-provider-testing.md.
There was a problem hiding this comment.
Fixed in 0cb0130. Removed the TODO and updated the guide: the host supplies the default chronological order, while the provider must honor the explicit order it receives before limit/offset. The existing grain and order-limit witnesses cover that obligation.
I also addressed the documentation notes from your review body: the plugin-loading example uses -p before collection, the grain wording matches the enforced witness, and the isolation diagnostic identifies transitive framework imports. The value/count success gaps are real, including the host's existing get_values shape expectation. I suggest a separate follow-up for success fixtures and filtered-value assertions; this patch keeps those methods scoped to error propagation. The SDK suite passes with 64 tests and four skips for unadvertised capabilities.
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Two things to fix before merge. The grouped-OR/NULL case does not use the filter shape the host actually sends, and the ADHOC_COLUMN_EXPRESSIONS lane cannot be met without breaking the SDK's own types. Both are part of the fixture contract that providers will code against, so fix them before this ships as public SDK surface.
I ran the suite on 0cb0130 (43 passed, 4 skipped). Then I mutated conformance_provider.py one change at a time and re-ran TestExampleConformance:
| Mutation | Result |
|---|---|
ignore offset |
caught |
| ignore grain (group by raw day) | caught |
| drop the parentheses around the OR group | passes |
drop every top-level comparison filter (=, >=, <, IN, ...); keep only IS [NOT] NULL and OR groups |
passes |
return a column-less pa.table({}) when no rows match |
passes |
| return an empty catalog for generation B | passes |
metadata_cache_token always None |
passes |
Packaging is fine. testing.py is the only module that imports pytest, nothing in superset_core.semantic_layers imports it, and [testing] is the extra.
Blocking
- The OR case doesn't test the shape the host sends (
testing.py:134-135, example fixturetest_conformance.py:123-133).- The lane accepts any
OrFilter, and the example uses{IS_NULL, EQUALS "A"}. - The host never builds that group.
_normalize_membership_filterinsuperset/semantic_layers/mapper.pyturns a mixed selectionIN ("A", NULL)intoOrFilter({IN frozenset({"A"}), IS_NULL}). That is the path theOperatordocstring says "providers must render both branches of". - So a provider can mishandle
INinside the group and still pass. Examples: binding the frozenset as a single parameter, rendering only one member, or rejectingINoutright. The example provider rejectsINwithINVALID_FILTER(conformance_provider.py:198-201) and passes the whole suite. - Precedence also goes unchecked, as noted in the existing thread. I confirmed it: dropping the parentheses passes.
- Fix: require an
INleaf with at least two non-NULL values in the group, plus an outer AND predicate whose result differs when the parentheses are lost. The doc currently only asks for the outer AND "when the provider supports" it (semantic-layer-provider-testing.md:65-66); make the kit enforce it.
- The lane accepts any
ADHOC_COLUMN_EXPRESSIONScan't be met (testing.py:214-217).SemanticQuery.dimensionsislist[Dimension].- The host rejects ad-hoc dimensions:
mapper.pyraises "Adhoc dimensions are not supported in Semantic Views." - A provider advertising this feature can only pass by building a query that violates the SDK type. Fixing that later would change public fixture expectations.
- Fix: skip this feature with an explicit "no SDK query shape yet" reason until the SDK defines one. Keep the
ADHOC_EXPRESSIONS_IN_ORDERBYcheck, which matches the host's_get_order_from_query_object.
Non-blocking
- No time-range or plain comparison filter case (
testing.py:103-105). Every time-series chart sendsGREATER_THAN_OR_EQUAL/LESS_THANon the temporal column (mapper.py, time-range filters). Every filter-box selection sendsIN. A provider that silently drops both passes (row 4 above). This list of case keys is effectively the fixture API, and adding required keys later breaks every subclass. Consider addinginandtime_rangecases now. - Empty results are never checked (
testing.py:116-118). Requiring nonempty expected tables is right for the discriminating cases. But nothing checks that a query matching no rows still returns one typed column per selected member, whichget_tablepromises ("Name each result column after the member that produced it"). The host relies on this when it selects columns by name from cached results (cache_transform.py:249). A filter matching nothing would cover it. - The metadata-generation check is weaker than its docstring (
testing.py:236-243).- The existing thread already covers
after != beforeaccepting an empty generation-B catalog. I confirmed that it passes. - Separately, a provider whose
metadata_cache_tokenis alwaysNonesatisfies all three token assertions, so "never relabeled" is vacuous for it. - If
Nonestays allowed for legacy views, say in the doc that the relabel check doesn't apply to them. Alternatively, add a fixture flag so a cached provider opts into strict token echo.
- The existing thread already covers
- A missing required case gives a bare
KeyError(testing.py:115,:163,:197). For example, a provider with no MONTH grain getsKeyError: 'grain'.pytest.fail(f"semantic_query_cases is missing required case {case!r}")would tell the author which fixture key is missing. - The override recipe for unsupported error cases is heavier than needed (
semantic-layer-provider-testing.md:87-93). The operation runs insidepytest.raises(Exception), andpytest.skipraises aBaseException. So callingpytest.skip(reason)from the fixture callable already skips just that case, without overriding the method or repeating bothparametrizedecorators. Documenting that would be simpler and harder to get wrong. - API stability.
SemanticProviderConformance, its fixture names and their tuple and mapping shapes are now publicapache-superset-core(0.1.0) API, and external providers subclass them. Add one sentence to the guide saying how new cases and fixtures will be introduced. For example: new required fixtures only in a minor version bump, with the case list in the changelog. Then provider lanes that pin the SDK know what an upgrade costs. - Nit.
pytestis unpinned in[testing]. The kit usespytest.MonkeyPatch.context(), which needs pytest ≥ 6.2. A lower bound would avoid confusing failures on old environments.
|
Following up on your review: Addressed the two pre-merge fixes in 06568a7. The OR witness uses the host-normalized IN/IS_NULL shape with two non-NULL membership values and an outer comparison. The example data exposes lost parentheses in either filter order, dropped top-level comparisons, and a dropped membership value. Those mutations have regressions against the real SQLite example. ADHOC_COLUMN_EXPRESSIONS skips with an explicit “no SDK query shape yet” reason; no query violating list[Dimension] is required. The representable ad-hoc ordering check stays in place. The generation check also requires an additive catalog, and the guide makes the None-token limitation explicit. The SDK suite passes with 77 tests and four explicit skips. I kept the existing required case keys for this patch. Dedicated IN/time-range/empty-result cases and the fixture-versioning policy need to be agreed together before adding required fixtures. The missing-case diagnostics, simpler skip recipe and pytest lower bound remain non-blocking follow-ups; the empty-result gap is not claimed fixed here. |
github-actions
Bot
added
the
requires:rebase
github-actions
Bot
removed
the
requires:rebase
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
LGTM. All prior threads are resolved in code (order/nullability, fault-lane programming errors, ad-hoc witnesses, generation-change catalog check, grouped-OR seed, grain skip). CI is green. Remaining non-blocking suggestions from fitzee's review (in/time_range cases, empty-result check, clearer missing-case failure, pytest lower bound) can be follow-ups.
SUMMARY
TL;DR: Provider authors get an opt-in pytest conformance suite that runs without a Superset application and checks observable results and failure categories.
Background: The semantic-layer SDK defines common query and metadata contracts, while each provider owns construction, transport and backend fixtures.
Problem: A provider can implement the right methods yet return the wrong grain, filter result, ordering or error category. Shared behavioral checks make those regressions easier to detect before integration.
What this changes:
SemanticProviderConformance, with provider-owned fixtures for deterministic queries, controlled errors and catalog changes.Depends on #45393. The conformance layer diff is available in the fork comparison.
The suite is optional test support; importing the SDK normally does not load pytest. Factory signatures, context types, production transports, host execution, and count/value success conventions are unchanged. Default chronological ordering remains a separate change in #45389. Provider integration lanes must use their pinned SDK and retain host and live-provider checks.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: SDK test support and documentation only.
TESTING INSTRUCTIONS
With the SDK dependencies and pytest installed, run from the repository root:
The example uses in-process SQLite, no server or credentials. Expected Arrow tables are independently specified. Controlled mutations were detected for NULL/OR behavior, grain handling, ordering, partial success, fault classification, stale catalogs and token relabeling. Negative import/schema tests also verify that catching a tripwire exception cannot make the check pass.
The import/schema tripwires are not a sandbox or exhaustive proof against native-driver I/O. Provider lanes must also poison their own transport and credential entry points. Passing these checks does not establish deployed host or live-warehouse conformance.
ADDITIONAL INFORMATION