Skip to content

Commit b54a789

Browse files
committed
fix(table_diff): resolve on/skip_columns case-insensitively against schema
On engines whose sqlglot dialect lowercases unquoted identifiers (e.g. BigQuery, DuckDB), TableDiff.key_columns normalized `on` column names via normalize_identifiers() and then looked them up verbatim in the schema returned by adapter.columns(), which preserves the table's actual column casing. When the two disagreed, this raised a bare KeyError before any query ran (table_diff.py:326), and in the single-key path caused a similar KeyError deeper in _fetch_sample. `skip_columns` had the same normalize-then-exact-match mismatch, so columns intended to be excluded silently were not. Add TableDiff._resolve_column_name() to fall back to a case-insensitive match against the schema when there's no exact match, and use it when building key_columns (for both the single- and multi-column paths) and when resolving skip_columns. Add regression tests reproducing the reported KeyError for both single- and multi-column `on` lists, plus a case-insensitive skip_columns test. Fixes #6067 Signed-off-by: Vaibhav Naik <naikvaib@amazon.com>
1 parent ad2377e commit b54a789

2 files changed

Lines changed: 116 additions & 11 deletions

File tree

sqlmesh/core/table_diff.py

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -255,14 +255,7 @@ def __init__(
255255
self.source_alias = source_alias
256256
self.target_alias = target_alias
257257

258-
cols: t.List[str] = ensure_list(skip_columns)
259-
self.skip_columns = {
260-
normalize_identifiers(
261-
exp.parse_identifier(col),
262-
dialect=self.model_dialect or self.dialect,
263-
).name
264-
for col in cols
265-
}
258+
self._skip_columns_raw: t.List[str] = ensure_list(skip_columns)
266259

267260
self._on = on
268261
self._row_diff: t.Optional[RowDiff] = None
@@ -275,16 +268,50 @@ def source_schema(self) -> t.Dict[str, exp.DataType]:
275268
def target_schema(self) -> t.Dict[str, exp.DataType]:
276269
return self.adapter.columns(self.target_table)
277270

271+
@cached_property
272+
def skip_columns(self) -> t.Set[str]:
273+
dialect = self.model_dialect or self.dialect
274+
names = set()
275+
for col in self._skip_columns_raw:
276+
normalized_name = normalize_identifiers(exp.parse_identifier(col), dialect=dialect).name
277+
# Resolve against both schemas (case-insensitively, if needed) so a column is
278+
# skipped even if the two tables disagree on casing, or the engine reports a
279+
# different case than the normalized `skip_columns` name.
280+
names.add(self._resolve_column_name(normalized_name, self.source_schema))
281+
names.add(self._resolve_column_name(normalized_name, self.target_schema))
282+
return names
283+
284+
@staticmethod
285+
def _resolve_column_name(name: str, schema: t.Dict[str, exp.DataType]) -> str:
286+
"""Resolves `name` to the corresponding key in `schema`.
287+
288+
Some dialects (e.g. BigQuery, DuckDB) normalize unquoted identifiers to a
289+
different case than what the engine's `adapter.columns()` reports for the
290+
underlying table (which reflects however the table was actually created).
291+
If there isn't an exact match, fall back to a case-insensitive lookup so
292+
the normalized `on`/`skip_columns` names still resolve to the real column.
293+
"""
294+
if name in schema:
295+
return name
296+
297+
for actual_name in schema:
298+
if actual_name.lower() == name.lower():
299+
return actual_name
300+
301+
return name
302+
278303
@cached_property
279304
def key_columns(self) -> t.Tuple[t.List[exp.Column], t.List[exp.Column], t.List[str]]:
280305
dialect = self.model_dialect or self.dialect
281306

282307
# If the columns to join on are explicitly specified, then just return them
283308
if isinstance(self._on, (list, tuple)):
284309
identifiers = [normalize_identifiers(c, dialect=dialect) for c in self._on]
285-
s_index = [exp.column(c, "s") for c in identifiers]
286-
t_index = [exp.column(c, "t") for c in identifiers]
287-
return s_index, t_index, [i.name for i in identifiers]
310+
s_names = [self._resolve_column_name(i.name, self.source_schema) for i in identifiers]
311+
t_names = [self._resolve_column_name(i.name, self.target_schema) for i in identifiers]
312+
s_index = [exp.column(name, "s") for name in s_names]
313+
t_index = [exp.column(name, "t") for name in t_names]
314+
return s_index, t_index, s_names
288315

289316
# Otherwise, we need to parse them out of the supplied "on" condition
290317
index_cols = []

tests/core/test_table_diff.py

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1246,3 +1246,81 @@ def test_data_diff_nulls_in_some_grain_columns():
12461246
"null value",
12471247
"null value modified",
12481248
]
1249+
1250+
1251+
def test_data_diff_on_columns_with_non_lowercase_names():
1252+
# On engines whose sqlglot dialect lowercases unquoted identifiers (e.g. BigQuery, DuckDB),
1253+
# `on` columns are normalized to lowercase before being looked up in the schema returned by
1254+
# `adapter.columns()`, which preserves the original (non-lowercase) casing. This used to raise
1255+
# a KeyError instead of resolving case-insensitively (issue #6067).
1256+
engine_adapter = DuckDBConnectionConfig().create_engine_adapter()
1257+
1258+
columns_to_types = {
1259+
"KEY1": exp.DataType.build("int"),
1260+
"KEY2": exp.DataType.build("int"),
1261+
"VALUE": exp.DataType.build("varchar"),
1262+
}
1263+
1264+
engine_adapter.create_table("src", columns_to_types)
1265+
engine_adapter.create_table("target", columns_to_types)
1266+
1267+
src_records = [(1, 1, "a"), (2, 2, "source only")]
1268+
target_records = [(1, 1, "a"), (3, 3, "target only")]
1269+
1270+
src_df = pd.DataFrame(data=src_records, columns=columns_to_types.keys())
1271+
target_df = pd.DataFrame(data=target_records, columns=columns_to_types.keys())
1272+
1273+
engine_adapter.insert_append("src", src_df)
1274+
engine_adapter.insert_append("target", target_df)
1275+
1276+
# multiple key columns, referenced with a case that doesn't match the schema
1277+
multi_key_diff = TableDiff(
1278+
adapter=engine_adapter, source="src", target="target", on=["key1", "key2"]
1279+
).row_diff()
1280+
1281+
assert multi_key_diff.full_match_count == 1
1282+
assert multi_key_diff.s_only_count == 1
1283+
assert multi_key_diff.t_only_count == 1
1284+
1285+
# single key column, referenced with a case that doesn't match the schema
1286+
single_key_diff = TableDiff(
1287+
adapter=engine_adapter, source="src", target="target", on=["KEY1"]
1288+
).row_diff()
1289+
1290+
assert single_key_diff.full_match_count == 1
1291+
assert single_key_diff.s_only_count == 1
1292+
assert single_key_diff.t_only_count == 1
1293+
1294+
1295+
def test_data_diff_skip_columns_with_non_lowercase_names():
1296+
# `skip_columns` goes through the same normalize-then-exact-match lookup as `on`, so it is
1297+
# subject to the same casing mismatch on engines that lowercase unquoted identifiers.
1298+
engine_adapter = DuckDBConnectionConfig().create_engine_adapter()
1299+
1300+
columns_to_types = {
1301+
"KEY1": exp.DataType.build("int"),
1302+
"IGNORE_ME": exp.DataType.build("varchar"),
1303+
"VALUE": exp.DataType.build("varchar"),
1304+
}
1305+
1306+
engine_adapter.create_table("src", columns_to_types)
1307+
engine_adapter.create_table("target", columns_to_types)
1308+
1309+
# IGNORE_ME differs between source and target, but should be excluded from comparison
1310+
src_df = pd.DataFrame(data=[(1, "src-only-value", "a")], columns=columns_to_types.keys())
1311+
target_df = pd.DataFrame(data=[(1, "target-only-value", "a")], columns=columns_to_types.keys())
1312+
1313+
engine_adapter.insert_append("src", src_df)
1314+
engine_adapter.insert_append("target", target_df)
1315+
1316+
diff = TableDiff(
1317+
adapter=engine_adapter,
1318+
source="src",
1319+
target="target",
1320+
on=["KEY1"],
1321+
skip_columns=["ignore_me"],
1322+
).row_diff()
1323+
1324+
assert diff.full_match_count == 1
1325+
assert diff.s_only_count == 0
1326+
assert diff.t_only_count == 0

0 commit comments

Comments
 (0)