Fix: generate_surrogate_key returns hex strings for SHA256/SHA512 on Presto and Trino

[Pawansingh3889]

Description

Fixes #5871.

@GENERATE_SURROGATE_KEY(..., hash_function := 'SHA256') produces a key that changes value depending on the engine. With MD5 the macro converts the parsed exp.MD5Digest into exp.MD5, so Presto and Trino render the hex-string form (LOWER(TO_HEX(MD5(TO_UTF8(...))))). With SHA256/SHA512 there is no equivalent conversion, so Trino gets a bare SHA256(varchar): a type error at execution time, and a binary digest rather than a hex string where it does run.

Two halves to the fix:

1. exp.SHA2Digest results convert to exp.SHA2 (keeping the length), mirroring the MD5 conversion one line above, and the concatenated argument is annotated as text so generators that wrap an encode around string inputs (TO_UTF8 on Presto/Trino) can do so. This is the half that matters once the sqlglot pin includes Fix(presto)!: preserve SHA256/SHA512 digest semantics, render SHA2 as hex string [CLAUDE] tobymao/sqlglot#7824 (merged upstream, not yet in a release).

2. A Presto-family fallback for the currently pinned sqlglot (~=30.8.0), where the trino/presto parsers hand back exp.SHA2 directly and render it as a bare SHA256(varchar). When a cached probe (_sha2_renders_binary) sees that the target dialect renders exp.SHA2 without TO_HEX, the macro builds the hex-string form itself, the same shape those generators produce for MD5. Once the pin moves past #7824 the probe turns the fallback off and the macro defers to the native rendering, so nothing double-wraps.

The repro from #5871 on this branch (trino):

SELECT LOWER(TO_HEX(SHA256(TO_UTF8(CONCAT(CAST(COALESCE(CAST(a AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR), CAST('|' AS VARCHAR), CAST(COALESCE(CAST(b AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR)))))) AS k FROM foo

Rendered SQL for duckdb, bigquery and snowflake is unchanged.

Test Plan

test_generate_surrogate_key_hash_semantics in tests/core/test_macros.py asserts: the macro returns exp.SHA2 (never a digest node) with a text-typed argument; the exact Trino and Presto SHA256 and SHA512 output strings (the reported fix); the fallback stays off for duckdb/bigquery and snowflake output is unchanged. The Trino/Presto string assertions hold on both sides of the sqlglot pin bump.

Full tests/core/test_macros.py: 139 passed. ruff check and ruff format clean on the changed files.

Checklist

• I have run make style and fixed any issues (ruff check and ruff format pass on the changed files; happy to fix anything else CI flags)
• I have added tests for my changes (if applicable)
• All existing tests pass (make fast-test) - ran tests/core/test_macros.py in full (139 passed); leaving the broader suite to CI
• My commits are signed off (git commit -s) per the DCO

[mday-io]

Thanks for picking this up. I think the direction makes sense, but this PR does not actually fix the reported Trino/Presto behavior with the currently pinned sqlglot~=30.8.0.

On this branch, this still renders the same invalid Trino SQL from #5871:

from sqlglot import parse_one
from sqlmesh.core.macros import MacroEvaluator

sql = "SELECT @GENERATE_SURROGATE_KEY(a, b, hash_function := 'SHA256') AS k FROM foo"
print(MacroEvaluator(dialect="trino").transform(parse_one(sql, dialect="trino")).sql("trino"))

Output is still:

SELECT SHA256(CONCAT(...VARCHAR...)) AS k FROM foo

Trino expects sha256(varbinary), and for surrogate-key semantics we need a string key, so the expected shape is:

LOWER(TO_HEX(SHA256(TO_UTF8(CONCAT(...)))))

The reason is that, on the current sqlglot pin, exp.func("SHA256", ..., dialect="trino") already returns exp.SHA2, not exp.SHA2Digest, so the new elif isinstance(func, exp.SHA2Digest) branch is not reached for Trino/Presto.

To make this PR complete, please do one of these:

1. Include the sqlglot bump that contains the Trino/Presto SHA2 rendering behavior, then add direct assertions for Trino and Presto SHA256/SHA512 output.
2. Or implement a SQLMesh-side fallback for affected dialects so SHA256/SHA512 render as LOWER(TO_HEX(<digest>(TO_UTF8(...)))) under the current sqlglot pin.

Either way, the test should assert the actual reported fix, for example that Trino @GENERATE_SURROGATE_KEY(..., hash_function := 'SHA256') renders LOWER(TO_HEX(SHA256(TO_UTF8(...)))), not only that BigQuery converts SHA2Digest to SHA2.

[Pawansingh3889]

Yes, still on it. Going with your option 2: a Presto/Trino-side fallback in the macro so SHA256/SHA512 render as LOWER(TO_HEX(SHA256(TO_UTF8(...)))) under the current sqlglot pin. The fallback is gated by a probe (render a throwaway exp.SHA2 for the target dialect and check for TO_HEX), so once a sqlglot release containing tobymao/sqlglot#7824 lands and the pin moves, the macro defers to the native rendering and never double-wraps. Tests will assert the exact Trino and Presto SHA256/SHA512 output strings as you specified.

[Pawansingh3889]

Pushed. Your repro on this branch now renders:

SELECT LOWER(TO_HEX(SHA256(TO_UTF8(CONCAT(CAST(COALESCE(CAST(a AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR), CAST('|' AS VARCHAR), CAST(COALESCE(CAST(b AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR)))))) AS k FROM foo

The macro builds the hex-string form itself for the Presto family under the current pin, mirroring those generators' MD5 handling. The fallback is gated by a cached probe (_sha2_renders_binary) that checks whether the dialect already renders exp.SHA2 with TO_HEX, so once the pin moves past tobymao/sqlglot#7824 the macro defers to the native rendering and nothing double-wraps. The test now asserts the exact Trino and Presto SHA256/SHA512 output strings as requested, plus that the fallback stays off for duckdb/bigquery/snowflake; those assertions hold unchanged on both sides of the pin bump. 139 macro tests pass, ruff check and format clean.

[mday-io]

Please run make style and any corrections needed

[Pawansingh3889]

Done — make style is clean now.

The failure was the mypy hook, not ruff:

tests/core/test_macros.py:1267: error: Item "list[Expr]" of "Expr | list[Expr] | None" has no attribute "sql"  [union-attr]
tests/core/test_macros.py:1267: error: Item "None" of "Expr | list[Expr] | None" has no attribute "sql"  [union-attr]

MacroEvaluator.transform returns Expr | t.List[Expr] | None. The surrounding tests in this file call .sql() on that union too, but they are unannotated, so mypy skips their bodies under the tests.* disallow_untyped_defs = false override. My test carries a -> None, so its body does get checked. Fixed by binding the result and narrowing with an isinstance assert rather than dropping the annotation.

SKIP=prettier,eslint pre-commit run --all-files now passes all four hooks (ruff, ruff-format, mypy, valid migrations) against .[dev,web,slack,dlt,lsp] on 3.12. tests/core/test_macros.py: 142 passed. Your repro is unchanged on this branch:

SELECT LOWER(TO_HEX(SHA256(TO_UTF8(CONCAT(CAST(COALESCE(CAST(a AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR), CAST('|' AS VARCHAR), CAST(COALESCE(CAST(b AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR)))))) AS k FROM foo

The workflow runs for this push are sitting in action_required — they need a maintainer to approve them before they'll start.

[mday-io]

The _sha2_renders_binary helper lists athena alongside presto/trino as needing the hex-string fallback:

if dialect_name not in ("presto", "trino", "athena"):
    return False

but the fallback never actually fires for Athena. exp.func("SHA256", ..., dialect="athena") returns exp.Anonymous (not exp.SHA2/exp.SHA2Digest like Presto/Trino), so neither the elif isinstance(func, exp.SHA2Digest) branch nor the final isinstance(func, exp.SHA2) check ever trips for Athena, and _sha2_renders_binary is never even called on that path.

I confirmed this against the pinned sqlglot~=30.8.0: rendered SQL for @GENERATE_SURROGATE_KEY(a, hash_function := 'SHA256') with dialect="athena" is identical before and after this PR — still a bare SHA256(CAST(...)). Since Athena runs on the Trino engine, this hits the same failure from #5871 (sha256 expects varbinary, not varchar), just not fixed for Athena. There's also no test exercising the Athena path, which is presumably why this slipped through.

Could you add an exp.Anonymous-based (or Presto-semantics) handling path for Athena's SHA256/SHA512, plus a test_generate_surrogate_key_hash_semantics case for dialect="athena" alongside the existing Trino/Presto assertions?

[Pawansingh3889]

Good catch, and confirmed — thank you. Fixed in f5e9b9a.

You were right about the mechanism: exp.func("SHA256", ..., dialect="athena") returns exp.Anonymous, so neither branch fired and _sha2_renders_binary was never reached on that path. Athena was in the family tuple but never actually fixed.

The macro now converts that Anonymous node to exp.SHA2, which puts Athena back on the existing machinery:

SELECT LOWER(TO_HEX(SHA256(TO_UTF8(CONCAT(CAST(COALESCE(CAST(a AS VARCHAR), '_sqlmesh_surrogate_key_null_') AS VARCHAR), ...))))) AS k FROM foo

One thing worth flagging, because it changes the lifetime of this code: the Athena branch is not a pin-era workaround the way the probe is. I checked against 30.14.0, and Athena still parses SHA256 to Anonymous there:

dialect	exp.func("SHA256") on 30.8.0	on 30.14.0
trino / presto	exp.SHA2	exp.SHA2Digest
athena	exp.Anonymous	exp.Anonymous

So when the pin moves the probe goes inert but this branch must stay, otherwise Athena regresses while Presto and Trino stay correct. I noted that in the commit message and the code comment so it does not get cleaned up later as dead pin-era code.

Also added the athena cases to test_generate_surrogate_key_hash_semantics alongside trino/presto, plus a MYHASH case: Anonymous is the catch-all for any unrecognised function name and hash_function is caller-supplied, so the conversion is keyed on the name rather than assumed — an unknown hash has to pass through untouched instead of being silently reinterpreted as SHA-256.

[mday-io]

hasattr(generate_surrogate_key, "func") is always False here. the @macro() decorator (registry_decorator.__call__) returns a plain @wraps-wrapped function, not an object exposing .func. So the if branch is dead and only else generate_surrogate_key ever runs. Could simplify to just:

from sqlmesh.core.macros import generate_surrogate_key

and call generate_surrogate_key(...) directly.

[Pawansingh3889]

You are right — hasattr(generate_surrogate_key, "func") is always False, so only the else ever ran. Confirmed:

>>> from sqlmesh.core.macros import generate_surrogate_key as g
>>> type(g).__name__, hasattr(g, "func")
("function", False)

Simplified to call the imported name directly, as suggested. Done in 33084d3.

[mday-io]

_sha2_renders_binary detects "does this sqlglot version already emit TO_HEX" by rendering a probe expression and substring-matching "TO_HEX" in the output. The codebase already has a more direct pattern for this exact kind of gating — SQLGLOT_VERSION_TUPLE in sqlmesh/utils/cache.py, and major_minor(SQLGLOT_VERSION) in sqlmesh/core/state_sync/db/version.py / migrator.py. A version-tuple check against the sqlglot release that fixed tobymao/sqlglot#7824 would be less fragile than string-matching rendered SQL, which could misfire if the Presto/Trino generator's output text changes for unrelated reasons in a future sqlglot release. Not a blocker, but might be worth aligning with the existing convention.

[Pawansingh3889]

Fair point on the convention, and I looked into it properly before answering. I have left the probe in place, and I want to show the reasoning rather than just assert it — happy to switch if you still prefer the version gate.

First, the fact that was missing: I bisected the releases, and the first sqlglot carrying #7824 is 30.13.0.

sqlglot	exp.SHA2(...).sql("trino")
30.8.0 – 30.12.0	SHA256(x)
30.13.0+	LOWER(TO_HEX(SHA256(x)))

So the gate would be < (30, 13, 0). The problem is that SQLGLOT_VERSION_TUPLE in utils/cache.py is built as tuple(SQLGLOT_VERSION.split(".")) — a tuple of strings — and aligning with it naively gives the wrong answer:

>>> SQLGLOT_VERSION_TUPLE                          # 30.8.0
("30", "8", "0")
>>> SQLGLOT_VERSION_TUPLE < ("30", "13", "0")
False        # but 30.8.0 IS older than 30.13.0
>>> "8" < "13"
False        # string compare, not numeric

It silently misfires at exactly the boundary this gate exists for: the fallback would switch off on the pinned version and emit the broken SHA256(varchar) again. Correct usage needs tuple(map(int, ...)), which is a different comparison from the one the existing call sites do — major_minor() only ever compares two components, so it has not hit this.

Beyond that, the probe answers the question the code actually needs. The macro does not care which version is installed; it cares whether this generator emits the hex form, and it asks that directly. That keeps it right for cases a version number gets wrong — a backport, a fork, an unreleased install, or a dialect that diverges from the family. Athena is a live example of a dialect not moving in lockstep with the version: it still parses to Anonymous on 30.14.0, so its handling is version-independent by necessity.

On the fragility you raised — if the Presto generator stopped emitting TO_HEX, the surrogate key would no longer be a hex string, so the substring is load-bearing rather than incidental. It is also cheap: @lru_cache means one render per dialect per process.

That said, this is your codebase and the convention argument is reasonable. If you would rather have the version gate, say so and I will switch it to tuple(map(int, SQLGLOT_VERSION.split(".")))[:3] < (30, 13, 0) — and it is probably worth fixing or documenting the string-tuple footgun in utils/cache.py separately, since the next person to reach for it will hit the same thing.

[Pawansingh3889]

Pushed f5e9b9a and 33084d3 — all three review comments addressed. Replies are on the individual threads; summary here.

Athena (the real bug). Confirmed and fixed. exp.func("SHA256", ..., dialect="athena") returns exp.Anonymous, so no branch fired and _sha2_renders_binary was never reached. The macro now converts that node to exp.SHA2, which puts Athena back on the existing path. Worth noting for later: this branch is not a pin-era workaround like the probe — Athena still parses to Anonymous on 30.14.0, so removing it when the pin moves would regress Athena while leaving Presto/Trino correct.

Dead hasattr branch. Correct, always False. Simplified to a direct call.

Version gate instead of the probe. Looked into it and left the probe in, with reasoning on the thread. Short version: the first sqlglot with #7824 is 30.13.0, and aligning with the existing SQLGLOT_VERSION_TUPLE naively is wrong, because it is a tuple of strings — ("30","8","0") < ("30","13","0") is False, so the fallback would switch off on the pinned version and re-emit the broken SQL. Happy to switch to an int-tuple gate if you prefer it.

Verification

• tests/core/test_macros.py: 142 passed, including new athena SHA256/SHA512 cases and a MYHASH case covering the name check (Anonymous is the catch-all, so an unknown hash_function must pass through untouched).
• SKIP=prettier,eslint pre-commit run --all-files: ruff, ruff-format, mypy, valid migrations all pass.
• Checked the design against 30.14.0 at the sqlglot level: the probe correctly reads TO_HEX, turns the fallback off, and the native rendering produces the same shape — no double-wrap. (The full suite cannot run there; sqlmesh’s parser patches are incompatible with 30.14, which is what the ~=30.8.0 pin is for.)

CI is still action_required and needs a maintainer to approve the workflow runs.

[mday-io]

Thanks @Pawansingh3889 - looks good!