Skip to content

Commit 13787d7

Browse files
albertosuman-1k5albertosuman-1k5
authored
fix: non-breaking detection for redundant cast projections (#5851)
Signed-off-by: Alberto Suman <alberto.suman@1komma5grad.com>
1 parent 9a25aa1 commit 13787d7

2 files changed

Lines changed: 271 additions & 2 deletions

File tree

‎sqlmesh/core/model/definition.py‎

Lines changed: 94 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1594,7 +1594,7 @@ def is_breaking_change(self, previous: Model) -> t.Optional[bool]:
15941594

15951595
for edit in edits:
15961596
if not isinstance(edit, Insert):
1597-
return None
1597+
return _additive_projection_change(previous_query, this_query, self.dialect)
15981598

15991599
expr = edit.expression
16001600
if isinstance(expr, exp.UDTF):
@@ -1608,7 +1608,7 @@ def is_breaking_change(self, previous: Model) -> t.Optional[bool]:
16081608
expr = parent
16091609

16101610
if not _is_projection(expr) and expr.parent not in inserted_expressions:
1611-
return None
1611+
return _additive_projection_change(previous_query, this_query, self.dialect)
16121612

16131613
return False
16141614

@@ -2913,6 +2913,98 @@ def _is_projection(expr: exp.Expr) -> bool:
29132913
return isinstance(parent, exp.Select) and expr.arg_key == "expressions"
29142914

29152915

2916+
def _has_ordinal_references(query: exp.Select) -> bool:
2917+
order = query.args.get("order")
2918+
if order and any(
2919+
isinstance(ob.this, exp.Literal) and ob.this.is_number for ob in order.expressions
2920+
):
2921+
return True
2922+
group = query.args.get("group")
2923+
return bool(
2924+
group and any(isinstance(gb, exp.Literal) and gb.is_number for gb in group.expressions)
2925+
)
2926+
2927+
2928+
def _additive_projection_change(
2929+
previous_query: exp.Query,
2930+
this_query: exp.Query,
2931+
dialect: DialectType,
2932+
) -> t.Optional[bool]:
2933+
"""Fallback for when SQLGlot's tree diff can't express an additive projection change.
2934+
2935+
SQLGlot's diff matches nodes by structural similarity, so interchangeable leaves (e.g. two
2936+
identical ``CAST(... AS T)`` target types) can be cross-matched. Inserting a same-type cast
2937+
above an existing one therefore yields spurious ``Move`` / ``Update`` edits even though a
2938+
column was simply added to the SELECT list. In that case the edit-based check above is
2939+
inconclusive, so we verify additivity directly against the output projections.
2940+
2941+
Returns ``False`` (non-breaking) only when the change is provably additive:
2942+
* both queries are simple ``SELECT`` statements,
2943+
* everything other than the projection list is structurally identical,
2944+
* no added projection is a (potentially cardinality-changing) ``UDTF``,
2945+
* every previous projection is preserved, in order, within the new projection list, and
2946+
* no mid-list insert shifts ordinal ``ORDER BY`` / ``GROUP BY`` references.
2947+
2948+
Otherwise returns ``None`` (undetermined), preserving the conservative default.
2949+
"""
2950+
# UNIONs or other query expressions, are left to the caller's conservative diff result.
2951+
if not isinstance(previous_query, exp.Select) or not isinstance(this_query, exp.Select):
2952+
return None
2953+
2954+
previous_projections = previous_query.expressions
2955+
this_projections = this_query.expressions
2956+
# If the new query has not gained any projections, this cannot be an additive projection-only
2957+
# change, so there is nothing for this fallback to prove.
2958+
if len(this_projections) <= len(previous_projections):
2959+
return None
2960+
2961+
# Adding a UDTF projection (e.g. EXPLODE / UNNEST) can change row cardinality, so such a
2962+
# change is not safely non-breaking even when it appears as an extra SELECT item.
2963+
for projection in this_projections:
2964+
bare = projection.this if isinstance(projection, exp.Alias) else projection
2965+
if isinstance(bare, exp.UDTF):
2966+
return None
2967+
2968+
# Everything other than the projection list must be structurally identical. Replacing each
2969+
# SELECT list with the same dummy literal lets the expression equality check focus on the
2970+
# FROM / WHERE / GROUP BY / ORDER BY / etc. parts of the query.
2971+
previous_skeleton = previous_query.copy()
2972+
this_skeleton = this_query.copy()
2973+
previous_skeleton.set("expressions", [exp.Literal.number(1)])
2974+
this_skeleton.set("expressions", [exp.Literal.number(1)])
2975+
if previous_skeleton != this_skeleton:
2976+
return None
2977+
2978+
# Every previous projection must appear, in order, within the new projection list. Comparing
2979+
# dialect-normalized SQL makes semantically equivalent projection nodes match even when the
2980+
# parser built distinct object identities.
2981+
this_projection_sql = [p.sql(dialect=dialect, comments=False) for p in this_projections]
2982+
search_start = 0
2983+
matched_at: list[int] = []
2984+
for projection in previous_projections:
2985+
target_sql = projection.sql(dialect=dialect, comments=False)
2986+
# Continue after the previous match so added columns can appear before, between, or after
2987+
# the original projections, but existing projections cannot be reordered or rewritten.
2988+
for index in range(search_start, len(this_projection_sql)):
2989+
if this_projection_sql[index] == target_sql:
2990+
matched_at.append(index)
2991+
search_start = index + 1
2992+
break
2993+
else:
2994+
return None
2995+
2996+
# Mid-list inserts shift ordinal references in ORDER BY / GROUP BY clauses.
2997+
if _has_ordinal_references(this_query):
2998+
matched_set = set(matched_at)
2999+
last_matched = matched_at[-1]
3000+
if any(i < last_matched for i in range(len(this_projections)) if i not in matched_set):
3001+
return None
3002+
3003+
# At this point the query shape is unchanged and all prior outputs are preserved, so the only
3004+
# remaining difference is one or more additional, non-UDTF projections.
3005+
return False
3006+
3007+
29163008
def _single_expr_or_tuple(values: t.Sequence[exp.Expr]) -> exp.Expr | exp.Tuple:
29173009
return values[0] if len(values) == 1 else exp.Tuple(expressions=values)
29183010

‎tests/core/test_snapshot.py‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1633,6 +1633,183 @@ def test_categorize_change_sql(make_snapshot):
16331633
)
16341634

16351635

1636+
def test_categorize_change_sql_redundant_cast(make_snapshot):
1637+
# Adding a column with a redundant CAST above an existing same-type cast makes SQLGlot's tree
1638+
# diff emit spurious Move/Update edits (interchangeable DataType leaves get cross-matched),
1639+
# even though the change is purely an added projection. These must remain NON_BREAKING.
1640+
config = CategorizerConfig(sql=AutoCategorizationMode.SEMI)
1641+
1642+
old_snapshot = make_snapshot(
1643+
SqlModel(name="a", query=parse_one("SELECT a::DATE, s::TEXT FROM t"))
1644+
)
1645+
1646+
# A same-type cast column has been inserted mid-list, above the existing one.
1647+
assert (
1648+
categorize_change(
1649+
new=make_snapshot(
1650+
SqlModel(name="a", query=parse_one("SELECT a::DATE, x::TEXT, s::TEXT FROM t"))
1651+
),
1652+
old=old_snapshot,
1653+
config=config,
1654+
)
1655+
== SnapshotChangeCategory.NON_BREAKING
1656+
)
1657+
1658+
# Multiple same-type cast columns have been inserted mid-list.
1659+
assert (
1660+
categorize_change(
1661+
new=make_snapshot(
1662+
SqlModel(
1663+
name="a", query=parse_one("SELECT a::DATE, x::TEXT, s::TEXT, y::INT FROM t")
1664+
)
1665+
),
1666+
old=old_snapshot,
1667+
config=config,
1668+
)
1669+
== SnapshotChangeCategory.NON_BREAKING
1670+
)
1671+
1672+
# The type of an existing projection has changed (in addition to a new column): undetermined.
1673+
assert (
1674+
categorize_change(
1675+
new=make_snapshot(
1676+
SqlModel(name="a", query=parse_one("SELECT a::INT, x::TEXT, s::TEXT FROM t"))
1677+
),
1678+
old=old_snapshot,
1679+
config=config,
1680+
)
1681+
is None
1682+
)
1683+
1684+
# Existing projections have been reordered: undetermined.
1685+
assert (
1686+
categorize_change(
1687+
new=make_snapshot(
1688+
SqlModel(name="a", query=parse_one("SELECT s::TEXT, a::DATE FROM t"))
1689+
),
1690+
old=old_snapshot,
1691+
config=config,
1692+
)
1693+
is None
1694+
)
1695+
1696+
# An existing projection has been removed: undetermined.
1697+
assert (
1698+
categorize_change(
1699+
new=make_snapshot(SqlModel(name="a", query=parse_one("SELECT s::TEXT FROM t"))),
1700+
old=old_snapshot,
1701+
config=config,
1702+
)
1703+
is None
1704+
)
1705+
1706+
# A WHERE clause changed alongside the added cast column: undetermined.
1707+
assert (
1708+
categorize_change(
1709+
new=make_snapshot(
1710+
SqlModel(
1711+
name="a",
1712+
query=parse_one("SELECT a::DATE, x::TEXT, s::TEXT FROM t WHERE a = 2"),
1713+
)
1714+
),
1715+
old=make_snapshot(
1716+
SqlModel(name="a", query=parse_one("SELECT a::DATE, s::TEXT FROM t WHERE a = 1"))
1717+
),
1718+
config=config,
1719+
)
1720+
is None
1721+
)
1722+
1723+
# Mid-list insert with ORDER BY ordinal shifts the referenced projection: undetermined.
1724+
assert (
1725+
categorize_change(
1726+
new=make_snapshot(
1727+
SqlModel(
1728+
name="a",
1729+
query=parse_one("SELECT a::DATE, x::TEXT, s::TEXT FROM t ORDER BY 2"),
1730+
)
1731+
),
1732+
old=make_snapshot(
1733+
SqlModel(name="a", query=parse_one("SELECT a::DATE, s::TEXT FROM t ORDER BY 2"))
1734+
),
1735+
config=config,
1736+
)
1737+
is None
1738+
)
1739+
1740+
# Append at end with ORDER BY ordinal leaves existing ordinal bindings unchanged.
1741+
assert (
1742+
categorize_change(
1743+
new=make_snapshot(
1744+
SqlModel(
1745+
name="a",
1746+
query=parse_one("SELECT a::DATE, s::TEXT, x::TEXT FROM t ORDER BY 2"),
1747+
)
1748+
),
1749+
old=make_snapshot(
1750+
SqlModel(name="a", query=parse_one("SELECT a::DATE, s::TEXT FROM t ORDER BY 2"))
1751+
),
1752+
config=config,
1753+
)
1754+
== SnapshotChangeCategory.NON_BREAKING
1755+
)
1756+
1757+
# Mid-list insert with GROUP BY ordinal shifts the referenced projection: undetermined.
1758+
assert (
1759+
categorize_change(
1760+
new=make_snapshot(
1761+
SqlModel(
1762+
name="a",
1763+
query=parse_one("SELECT a::DATE, x::TEXT, s::TEXT FROM t GROUP BY 2"),
1764+
)
1765+
),
1766+
old=make_snapshot(
1767+
SqlModel(name="a", query=parse_one("SELECT a::DATE, s::TEXT FROM t GROUP BY 2"))
1768+
),
1769+
config=config,
1770+
)
1771+
is None
1772+
)
1773+
1774+
# Aliased UDTF projection via fallback path: undetermined.
1775+
assert (
1776+
categorize_change(
1777+
new=make_snapshot(
1778+
SqlModel(
1779+
name="a",
1780+
query=parse_one(
1781+
"SELECT a::DATE AS a, x::TEXT AS x, EXPLODE(y) AS y, s::TEXT AS s FROM t"
1782+
),
1783+
)
1784+
),
1785+
old=make_snapshot(
1786+
SqlModel(name="a", query=parse_one("SELECT a::DATE AS a, s::TEXT AS s FROM t"))
1787+
),
1788+
config=config,
1789+
)
1790+
is None
1791+
)
1792+
1793+
# UDTF inside aliased scalar subquery remains non-breaking.
1794+
assert (
1795+
categorize_change(
1796+
new=make_snapshot(
1797+
SqlModel(
1798+
name="a",
1799+
query=parse_one(
1800+
"SELECT a::DATE AS a, (SELECT x FROM unnest(b) x) AS sub, s::TEXT AS s FROM t"
1801+
),
1802+
)
1803+
),
1804+
old=make_snapshot(
1805+
SqlModel(name="a", query=parse_one("SELECT a::DATE AS a, s::TEXT AS s FROM t"))
1806+
),
1807+
config=config,
1808+
)
1809+
== SnapshotChangeCategory.NON_BREAKING
1810+
)
1811+
1812+
16361813
def test_categorize_change_seed(make_snapshot, tmp_path):
16371814
config = CategorizerConfig(seed=AutoCategorizationMode.SEMI)
16381815
model_name = "test_db.test_seed_model"

0 commit comments

Comments
 (0)