Skip to content

A statement the warehouse refuses is classified, names its object, and reports what it spent; profiling a non-PII string column no longer kills the statement on Redshift - #318

Merged
marcociav-exmergo merged 4 commits into
mainfrom
issue-307
Aug 15, 2026

Conversation

@spascarella

Copy link
Copy Markdown
Collaborator

What this changes

explore map --confirm and explore profile --confirm fail against Redshift on
any table with a non-PII string column, and have since 1.6.2. Three defects sit
on that path and each one hid the next. This fixes all three.

1. The profiling aggregate no longer depends on lazy CASE evaluation

type_contradiction_expressions guarded every CAST behind a length-bounded
shape predicate inside a CASE, on the premise that a dialect without
TRY_CAST would then never evaluate the cast for a row the WHEN excluded.
Postgres honors that premise. Redshift does not: it evaluates the branch for
rows it never selects, so one ordinary varchar status or category column was
enough to fail the whole statement with Invalid digit, Value 'p', Pos 0, Type: Long.

Every CAST the probe builds is now total. Its argument is a CASE that
yields a digit-only string on every row, so no evaluation order can reach a
cast with something it cannot parse:

CAST(CASE WHEN "status" ~ '^-?[0-9]{1,10}$' THEN "status" ELSE '0' END AS BIGINT)

The sentinel is rejected by every predicate built on the cast (the epoch ranges
start in 2000, the slash-component test asks for > 12), so a row that reaches
the cast only because the cast is unconditional is never counted as evidence.
The "shaped versus not shaped" distinction the fractions' denominators need
moved to a separate uncast expression, which carries a string and so cannot
raise. One change in the shared expression builder covers all six adapters and
removes the standing assumption about lazy CASE evaluation everywhere.

Measurements are unchanged, denominators included.

2. A statement the warehouse refuses is now classified and named

A server-side SQL error escaped the adapters untranslated. Not being a
DexError, it fell through every entry in _reason_overrides() and arrived as
reason: internal ("not a deliberate dex refusal") with data: {}. Same class
of misclassification as #252.

Every adapter now raises a typed WarehouseQueryError, exported from the
package root and mapped to reason: execution_failure, carrying the server's
own message and error code trimmed to one line and capped. Redshift reads M
and C out of the driver payload rather than stringifying the whole dict.
Postgres gates on sqlstate and Databricks on ServerOperationError, so a
connection that died mid-statement is not reported as SQL the server rejected.
Profiling attaches the object name at the one place that knows it, since a map
run has a dozen statements in flight.

3. A failed billed command reports what it spent

_record_elapsed runs in a finally, so the wasted compute reached
.dex/spend.jsonl correctly, but the error envelope carried data: {}. On a
metered connector a caller reading only the envelope saw a failure that
appeared to cost nothing. Any error envelope now reports the settled spend,
as the budget-exhaustion path already did. Nothing billed means no spend block,
so a refusal that never reached the warehouse still says nothing rather than
claiming a zero.

Also

The live suites assert through a new assert_ok(rc, envelope) helper that
prints errors, reason, and warnings. assert rc == 0, envelope is why
this took sixteen CI runs to diagnose: pytest elides the middle of a long
assertion message, and the middle of a profiling envelope is where errors
sits, so the Redshift message never once reached a CI log. 35 call sites across
all five connectors.

Evidence

The offline suite passed before this change too, so the tests are the argument
here as much as the code is.

The fix reproduces the bug offline. A new test lifts each CAST argument
out of the generated statement and casts it over every row unconditionally,
which is exactly the work Redshift does eagerly. Under the old shape it raises
on DuckDB; under the new one it cannot raise anywhere. Alongside it,
assert_every_cast_is_total asserts the structural invariant on all six
adapters' generated SQL. Reverting the expression change fails 7 tests.

The fix is behavior-preserving. Old and new expressions were run side by
side on DuckDB across 12 fixtures (pure text, mixed epoch and text, MDY, DMY,
ambiguous slash dates, boundary epochs, all-null, empty table). All 14 aliases
agree on every fixture. End to end, dex demo plus explore map produces a
cache byte-identical to the pre-change run apart from updated_at, including
both of the demo warehouse's designed type-contradiction findings.

The envelope defects are pinned. A Redshift adapter test drives a real
Invalid digit payload through profiling and asserts the failure is typed,
carries the message and SQLSTATE, names the object, and still bills its
seconds to the ledger. A CLI-level test drives a refusal mid-profile on a
metered connector and asserts reason, the server's words, the object, and
data.spend, with its counterpart asserting that a refusal which never reached
the warehouse reports no spend at all.

Not done here

This needs the live Redshift suite to confirm it. I have no credentials for
that workgroup, so the fix is unverified against a real Redshift. The offline
evidence above is as close as it gets. The residual risk is narrow: it is that
Redshift also pushes a cast down into CASE branches, which would defeat the
sentinel shape too. That is not its documented behavior and this is the
standard workaround for it, but only a green Redshift job proves it.

Separately, and left alone deliberately: epoch_seconds_fraction is measured
over the epoch-shaped rows, so a column that is half digits and half pending
reports "100% of values fall in the plausible unix-epoch range". That reads
wrong, but it is pre-existing and identical before and after this change, and
moving a detection threshold is not this fix's job. Worth its own issue.

Related issues

Closes #310.

Checklist

  • Tests pass (uv run pytest in packages/dex-core): 2197 passed, 65
    skipped. The 4 deselected test_packaging.py cases install wheels from
    PyPI and GitHub and cannot run on this machine (TLS trust store); the
    other 13 packaging tests pass.
  • Eval scoring-core tests pass (uvx pytest evals): 26 passed
  • If a safety path is touched, the spine still holds: read-only against
    data, cost-guarded, PII flagged not surfaced, propose-don't-impose. The
    generated SQL is still SELECT-only in every dialect, the aggregates still
    emit fractions rather than values, and spend now appears on more
    envelopes rather than fewer.
  • CHANGELOG.md updated under [Unreleased]
  • Prose is em-dash free (checked in CI)
  • No credentials, secrets, or raw warehouse rows in code, tests, or
    fixtures. One judgment call worth review: WarehouseQueryError passes the
    server's message through to the envelope, and a server message can quote
    the fragment of a value that offended it (Invalid digit, Value 'p').
    That matches what an agent SQL failure already surfaces, and suppressing
    it is what made this class of failure undiagnosable. The class docstring
    states the tradeoff.

spascarella and others added 4 commits August 15, 2026 18:35
…column

The declared-type-vs-content probe (#204) guarded each CAST behind a
length-bounded shape predicate inside a CASE, on the premise that a dialect
without TRY_CAST would then never evaluate the cast for a row the WHEN
excluded. Postgres honors that premise. Redshift does not: it evaluates the
branch for rows it never selects, so one ordinary varchar status or category
column failed the whole profiling statement server-side with
"Invalid digit, Value 'p', Pos 0, Type: Long".

Every CAST the probe builds is now total. Its argument is a CASE yielding a
digit-only string on every row, with a sentinel where the shape predicate does
not match, so correctness is a property of the expression rather than of the
engine's evaluation order. The sentinel is rejected by every predicate built on
the cast, and the "shaped versus not shaped" distinction the denominators need
moves to a separate uncast expression, which carries a string and cannot raise.

Measurements are unchanged, denominators included: old and new expressions were
run side by side on DuckDB over twelve fixtures (pure text, mixed epoch and
text, MDY, DMY, ambiguous slash dates, boundary epochs, all-null, empty) and
every alias agrees, and `dex demo` plus `explore map` produces a byte-identical
cache.

Two tests, because the offline suite passed before this change too. The first
lifts each CAST argument out of the generated statement and casts it over every
row, which is exactly the work Redshift does eagerly, so the bug reproduces
offline. The second asserts the structural invariant on all six adapters'
generated SQL. Reverting the expression change fails seven tests.

Refs #310.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A SQL error from the warehouse escaped the adapters untranslated. Not being a
DexError, it fell through every entry in `_reason_overrides()` and arrived as
`reason: internal` ("not a deliberate dex refusal") with no data, no object
named, and the server's own diagnosis buried in a driver repr. That is the same
misclassification as #252, on the path where a live warehouse tells dex exactly
what is wrong.

`WarehouseQueryError` is that outcome, typed: exported from the package root
and classified `execution_failure`, the bucket a failed dbt run already uses,
because the statement ran and the server refused it. Each adapter raises it for
what it cannot translate into something more specific, so the reason reads the
same from every connector, and each one keeps its own narrow test for what
counts: Redshift and Snowflake off their ProgrammingError, Postgres off a
sqlstate (a connection that died mid-statement stays untyped rather than being
reported as SQL the server rejected), Databricks off ServerOperationError,
BigQuery off a BadRequest that is not the byte cap, DuckDB off duckdb.Error.

The message carries the server's words and error code, trimmed to one line and
capped, since a driver will happily append the statement or its whole error
payload. Redshift reads M and C out of that payload rather than stringifying
the dict.

The adapter knows what the server said; only the caller that built the
statement knows what it was about, so profiling attaches the object name on the
way past. A map run has a dozen statements in flight, and "the warehouse
refused a statement dex built" is actionable only when it names one.

Refs #310.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_record_elapsed` runs in a finally, so a statement that died mid-scan billed
its seconds to `.dex/spend.jsonl` correctly. The envelope said otherwise: only
the two-phase refusals carried spend, so every other error came back with
`data: {}` and a caller reading the envelope alone saw a failure that appeared
to cost nothing, on a metered connector. That is the wrong side of "cost
surfaced before any spend".

`settled_spend` is the settlement `stamp_spend` already did, split out for the
path that has an exception instead of a Result to stamp. The CLI reads it on
the way out, where the paradigm is already read for the same reason (close()
drops the adapter the gate hangs off), and fills it into any error envelope
that does not carry one, so a handler's richer partial-completion payload still
wins. Reading it is best-effort: a ledger that cannot be read must not replace
the exception in flight with a bookkeeping failure.

Nothing billed reports nothing. A refusal that stopped before the first
statement is free, and a spend block of zeroes on it would read as a claim
about money where silence is the honest answer, so both directions have a test.

Refs #310.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`assert rc == 0, envelope` is the obvious spelling and it is why the Redshift
failure in #310 cost sixteen CI runs to diagnose instead of five minutes:
pytest elides the middle of a long assertion message, and the middle of a
profiling envelope is exactly where `errors` sits, so the server's message
never once reached a CI log.

`assert_ok` prints the errors, the reason, and the warnings, and nothing else,
so the line that survives is the one worth reading. Applied across all five
connectors' live suites.

Refs #310.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@marcociav-exmergo marcociav-exmergo changed the title Issue 307 A statement the warehouse refuses is classified, names its object, and reports what it spent; profiling a non-PII string column no longer kills the statement on Redshift Aug 15, 2026

@marcociav-exmergo marcociav-exmergo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! nice breakdown

@marcociav-exmergo
marcociav-exmergo merged commit 2bf5f8d into main Aug 15, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The skill wrappers require uv but never say so, and fail with a raw traceback when it is missing

2 participants