Fix: generate_surrogate_key returns hex strings for SHA256/SHA512 on Presto and Trino#5888
Fix: generate_surrogate_key returns hex strings for SHA256/SHA512 on Presto and Trino#5888Pawansingh3889 wants to merge 4 commits into
Conversation
…Presto and Trino Signed-off-by: Pawan Singh Kapkoti <42340841+Pawansingh3889@users.noreply.github.com>
a6384fe to
f580eb3
Compare
|
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 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 fooTrino expects LOWER(TO_HEX(SHA256(TO_UTF8(CONCAT(...)))))The reason is that, on the current sqlglot pin, To make this PR complete, please do one of these:
Either way, the test should assert the actual reported fix, for example that Trino |
|
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. |
Bare SHA256(varchar) is a type error on Trino and binary semantics on Presto, so the macro now builds LOWER(TO_HEX(SHA256(TO_UTF8(...)))) itself for the Presto family, mirroring those generators' MD5 handling. A cached probe checks whether the dialect already renders exp.SHA2 in the hex form, so once the sqlglot pin includes tobymao/sqlglot#7824 the macro defers to the native rendering and never wraps twice. Adds direct Trino and Presto SHA256/SHA512 output assertions that hold on both sides of the pin bump. Signed-off-by: Pawan Singh Kapkoti <42340841+Pawansingh3889@users.noreply.github.com>
|
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 fooThe 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 ( |
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 parsedexp.MD5Digestintoexp.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 bareSHA256(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:
exp.SHA2Digestresults convert toexp.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_UTF8on 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).A Presto-family fallback for the currently pinned sqlglot (
~=30.8.0), where the trino/presto parsers hand backexp.SHA2directly and render it as a bareSHA256(varchar). When a cached probe (_sha2_renders_binary) sees that the target dialect rendersexp.SHA2withoutTO_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):
Rendered SQL for duckdb, bigquery and snowflake is unchanged.
Test Plan
test_generate_surrogate_key_hash_semanticsintests/core/test_macros.pyasserts: the macro returnsexp.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 checkandruff formatclean on the changed files.Checklist
make styleand fixed any issues (ruff check and ruff format pass on the changed files; happy to fix anything else CI flags)make fast-test) - ran tests/core/test_macros.py in full (139 passed); leaving the broader suite to CIgit commit -s) per the DCO