feat(salesforce): shared-engine support for contract checks on an org (R-642)

[Niels-b]

Shared-engine changes needed to run contract checks against a Salesforce org (R-642). The Salesforce
side is sodadata/soda-extensions#565.

Five production files, each pairing its change with tests that run on every data source, not
just Salesforce.

What changes for other data sources

Four of these alter shared behaviour, so that is the part worth reviewing hardest:

Change	Effect elsewhere
The schema check reads columns through get_schema_check_columns_metadata()	A new seam over the existing accessor, so the same metadata and the same SQL for every source. The one delta is where the SQL is built: it moves from __init__ to execute(), so a builder that raises (sparkdf rejects >2 prefixes) now leaves one check NOT_EVALUATED instead of aborting the verification. Outcomes are otherwise identical to main, including for zero resolved columns. Databricks overrides the new seam so its accessor's swallow keeps serving the bulk sweep without silencing the schema check.
Dataset-level duplicate refuses when the source cannot hash	Flag defaults True, so no SQL source reaches the guard.
Sampling refusal guards on supports_row_sampling()	Defaults True, and only SalesforceSqlDialect overrides it across all 20 installed dialects.
supported_check_types gates check parsing	Defaults None — no restriction — so every existing source keeps running every registered check type. Only a source that declares a set is affected.
Empty check diagnostics no longer raise on Cloud publish	Strictly more permissive: an empty dict previously fell to raise SodaException("Unrecognized check result type"), and now returns None. A populated non-SodaCloudJsonable dict still raises exactly as before.

Why the correctness guards exist

Dataset-level duplicate reported every row as a duplicate. The count is derived as
row_count - distinct_count, the distinct count comes from a row hash, and a source that cannot
express one leaves it unmeasured — which convert_db_value coerces to 0. Measured against a
Salesforce fixture with zero duplicates: FAILED at 100%. reconciliation's duplicate_diff already
guards on this flag; the direct check did not.

The refusal resolves in parse_check, so no metric is built and the query the source cannot answer is
never sent. It shares one mechanism with the declared-check-type gate rather than adding a second.
Note the name: a dataset-level duplicate hashes even for columns: [one]
(_build_combined_hash_sql keeps STRING_HASH for a single column), so the message says
"dataset-level" rather than "multi-column" and points at the per-column form, which needs no hash —
with the caveat that it excludes missing values where this one counts NULL as a value.

Sampling was requested but unavailable. Two coordinated guards. The construction-time one is
load-bearing: without it .SAMPLE() reaches the dialect's _build_sample_sql, whose default raises
NotImplementedError inside CheckCollectionImpl.__init__ — before the execute loop exists to catch
anything — and the whole verification aborts. The execute-time one turns the refusal into
NOT_EVALUATED with a reason instead of a raised exception. data_source_supports_sampling answers only
the capability question; each call site pairs it with the "was sampling requested" half it already asks.

supported_check_types

A source that speaks something other than SQL can only answer part of the contract language. Until now
the unanswerable part failed deep inside execution, surfacing as whatever the source said — a raw
MALFORMED_QUERY, which reads like an engine defect rather than a boundary.

DataSourceImpl.supported_check_types is Optional[frozenset[str]], default None. A source that
declares a set gets checks outside it refused in CheckImpl.parse_check, before any metric or query
is built — that ordering is the point, because some check types run a user-written query and reaching
setup would send it to a source that cannot run it.

The gate is enforced in the shared CheckImpl.parse_check, so it covers every CheckCollectionImpl
subtype — contracts, data standards, metric monitoring — and every parser in the check_parsers
registry, group_by included. Only reconciliation escapes, via its own parse_check.

Three deliberate choices, each measured rather than assumed:

• NOT_EVALUATED, not EXCLUDED. EXCLUDED means the user deselected the check, which is a different
  statement from "you asked for this and are not getting it".
• Logged as an error, so the contract lands on ERROR (exit 3) rather than UNKNOWN (exit 0). Note
  UNKNOWN, not PASSED: _get_contract_verification_status reaches PASSED only via
  all(outcome == PASSED), so one NOT_EVALUATED already drops it. The reason to go further is
  permanence — unlike a NOT_EVALUATED from an unmeasured metric, which may succeed next run, this check
  will never evaluate on this source until someone edits the contract, and exit 0 would leave the
  contract quietly asserting less than it claims.
• Sources declare the whole supported set, not the gaps, so a check type added to soda-core is
  unsupported on such a source until someone has verified it there.

The Cloud-payload fix is what makes the refusal publishable at all.
_build_v4_diagnostics_check_type_json_dict dispatches partly on the check type and partly on the
result class: type-matched branches call .get() on the diagnostics, while the class-matched ones
(schema, freshness) fall through to an else-arm that accepted only None or a SodaCloudJsonable. So a
result with no diagnostics had no representation that worked for both — None crashed the first set with
an AttributeError, {} crashed the second with Unrecognized check result type. Both failure modes
were hit for real while building this.

Test additions

test_aggregate_check.py had no min or max case at all, and sum only in its filtered variant
— three of the four documented aggregate functions were asserted nowhere, for any source. Added, plus a
filtered max.

New test_sampling_unsupported.py covers both sampling guards by patching the capability flag, so it
runs everywhere. Mutation-checked: removing the construction-time guard fails 2 of its 3 tests.

New test_supported_check_types.py patches the capability onto whatever source the suite runs against,
so the behaviour is asserted on every source rather than only the one that declares a set. The publish
test is the one that catches both crash modes above — mutation-checked at 0 → 4 failures.

New soda-databricks/tests/unit/test_databricks_columns_metadata.py pins the seam that keeps Databricks
loud: its accessor still absorbs failures for bulk callers, and still lets them surface for the schema
check. Mutation-checked — deleting the override fails it.

test_duplicate_dataset_check was defended by an assertion that held for the wrong reason: it
asserted only the outcome and the absent number, so downgrading the reason to debug would have produced
a silent NOT_EVALUATED with a clean exit, which is exactly what the guard exists to prevent. It now
asserts the message via caplog, and that the message names the per-column alternative.

Verification

Postgres soda-tests/tests/integration	190 passed, 10 skipped
DuckDB soda-tests/tests/integration	179 passed, 17 skipped
sparkdf soda-tests/tests/integration	179 passed, 17 skipped
soda-tests/tests/unit + feature	1204 passed, 2 skipped
soda-databricks/tests/unit	37 passed

Every count was compared against the same suite on a clean tree, not against a remembered baseline.
The integration deltas are exactly the one test removed with the zero-columns error.

Reviewer notes

• No cross-source behaviour change. An earlier revision made zero resolved columns an error; that
  has been removed, so outcomes match main on every source. Routing the schema check through its own
  accessor is what remains, and Databricks overrides it so the routing does not quieten a metadata
  failure that used to be loud.
• supported_check_types gates every CheckCollectionImpl subtype, not contracts alone. A test in
  sodadata/soda-extensions#565 pins that data standards are covered — it lives there because
  soda-data-standard is packaged there.
• Not included, and open: a filter: on a query:-form check is silently ignored on every data source.
  Measured and documented, but fixing it changes a long-standing shared flow, so it was left out of this
  PR deliberately.

🤖 Generated with Claude Code

[Niels-b]

Dependency note: the Salesforce side is sodadata/soda-extensions#565, which imports SODA_FAILED_ROWS_CTE_NAME from this PR. Merge order is this PR first, then the extensions one.

🤖 Generated with Claude Code

[mivds]

Suggest extracting this to a helper function to avoid nesting even more

[Niels-b]

Both arms extracted: _execute_queries() and _log_sampling_refusal(). The branch is now two lines and reads as the decision it is. The nesting was this PR's doing — the loop used to sit directly under the elif — so it seemed right to take the suggestion rather than defend it.

[mivds]

This validates at check evaluation time, but hasn't the query already happened at this point?

[Niels-b]

Correct, and it was the most useful comment on the PR. The execute loop completes before the evaluate loop, so the row-hash query was being sent and refused before the guard ran.

Moved to parse_check via a new CheckImpl.unsupported_reason() hook, so no metric is built and nothing is sent. It also collapses the two refusal mechanisms you flag below into one, with the condition living on the check rather than the data source.

[mivds]

What about a table without columns? I guess most data sources may not support that, but apparently postgres does

[Niels-b]

Moot now — the zero-columns error is removed entirely (see m1n0's thread on the same block). Worth saying the question was a real one: Postgres does allow CREATE TABLE t(), and the guard would have called that ERROR.

[mivds]

Might be out of scope for this PR, but I started poking at the flow & number of properties we're creating here. supported_check_types, supports_row_hashing, supports_row_sampling all fall into this category, and there's probably more.

The design feels clunky with a data source having to define what checks it supports. Logic about what capabilities a check requires should live in the check IMO. On top of this property, we have duplicate check doing exactly that conditionally. It asserts supports_row_hashing, but only if there are multiple columns involved.

Claude came up with an approach of having both sides define capabilities:

 class CheckImpl:
     def required_capabilities(self) -> frozenset[Capability]: ...
     # multi-column duplicate returns {ROW_HASHING}; single-column returns empty

 class DataSourceImpl:
     def capabilities(self) -> frozenset[Capability]: ...

The engine does one thing, in parse_check, still before setup_metrics:

 missing = check_impl.required_capabilities() - data_source_impl.capabilities()
 if missing:
     # refuse: "duplicate check on (a, b) requires row hashing,
     #          which <source> does not support"

This seems like a better design to me than what we have today, but likely involves a bigger change

[Niels-b]

Agreed on the direction, and parse_check is the right seam.

This PR takes the first step: all refusals now resolve there, and the multi-column condition moved onto the check class — your "logic about what a check requires should live in the check" point. CheckImpl.unsupported_reason() is your required_capabilities() with a string instead of an enum. The rest is follow-up work.

One thing worth recording from doing it: hashing and sampling unify cleanly as capabilities, but supported_check_types does not. "SOQL cannot do group_by" is a check type nobody has verified against the source, not a capability a check can name a requirement for — forcing it into the same enum means inventing one capability per check type. That is the design question the follow-up has to answer, and it is why this is not a one-hour change.

[Niels-b]

As was discussed this morning; we'll should do that properly at a later time.

[m1n0]

Approving — guards are correctly scoped and every hook #565 overrides lands on the class core actually consults. Fix the description before merging though:

• "A contract against a non-existent dataset becomes loud" isn't what changes. That's already ERROR/exit 3 on main: row_count always runs an aggregation and a missing table throws there. Ran a schema-only contract on a missing table on both trees — ERROR both. The real flip is a queryable dataset whose metadata query resolves zero columns (prefix/casing mismatch — DuckDB with a 2-part DQN does it): UNKNOWN/exit 0 → ERROR/exit 3. Right call, but say that, and add a customer-facing-release-note block for it — there's none.
• The gate isn't contract-only. Data standards go through CheckCollectionImpl._parse_checks too, and group_by registers into the shared CheckImpl.check_parsers, so it's gated as well. Only recon's own parse_check escapes.
• Your comment about #565 importing SODA_FAILED_ROWS_CTE_NAME is stale since the revert.

[m1n0]

Not the case — this path was already ERROR on main via the row_count aggregation. The new case is zero columns on a queryable dataset.

[Niels-b]

You were right, and the change is removed rather than reworded. Outcomes now match main on every source, including zero resolved columns — so there is no behaviour change to release-note.

Removing it did expose one thing worth knowing: routing the schema check through get_columns_metadata() is still needed for a source whose metadata is not SQL, but Databricks overrides that accessor to absorb exceptions for the bulk-metadata sweep. So the routing alone would have quietened a metadata failure that used to be loud on that one source. get_schema_check_columns_metadata() separates the two audiences — the sweep keeps its empty list, the schema check sees the failure — and Databricks overrides it. Pinned by a mutation-checked unit test.

[m1n0]

Add the prefixes here — the failure mode is a schema/prefix mis-split and this line doesn't say where it looked. Also this now logs three ERROR lines together with the Actual columns are None pair from evaluate().

[Niels-b]

Prefixes added, on the surviving message — the line you commented on goes away with the zero-columns error.

The pile-up goes with it on that path: without the error, execute() returns a measurement carrying an empty list, so get_value yields [] rather than None and the evaluate() pair never fires. That path drops from three ERROR lines to zero. The pair still fires on the exception path, which is pre-existing on main, so I left it alone.

[m1n0]

Also gates data standards and group_by (shared check_parsers registry). Only recon escapes via its own parse_check.

[Niels-b]

Corrected — and it reaches further than either of us said. MonitoringImpl and MonitoringBackfillImpl in soda-metric-monitoring are CheckCollectionImpl subclasses with no parse_check of their own, so metric monitoring is gated too.

The docstring now states the rule once — every check type in the check_parsers registry, for every CheckCollectionImpl subtype — rather than listing kinds and getting the list wrong. A test pinning the data-standards half is in sodadata/soda-extensions#565, since soda-data-standard is packaged there.

[m1n0]

Re-ran the probe on the new head vs main: all four rows match, including zero resolved columns — so there's no behaviour change left to release-note. unsupported_reason() in parse_check is the right seam and the Databricks split keeps metadata failures loud. Approving.

Leftovers, none blocking: the CTE dependency comment on this PR is still up and still wrong; test_sampling_unsupported.py still calls caplog.set_level; the construction-time comment in base.py (~595) still describes the old sampling_is_unsupported framing.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Niels-b]

Thanks for the leftovers; both have been addressed 👍