Skip to content

fix(model): keep quoting when normalizing column_descriptions keys - #5959

Open
ReguiguiMohamed wants to merge 2 commits into
SQLMesh:mainfrom
ReguiguiMohamed:fix/column-descriptions-quoted-identifiers
Open

fix(model): keep quoting when normalizing column_descriptions keys#5959
ReguiguiMohamed wants to merge 2 commits into
SQLMesh:mainfrom
ReguiguiMohamed:fix/column-descriptions-quoted-identifiers

Conversation

@ReguiguiMohamed

Copy link
Copy Markdown

Fixes #5943.

_column_descriptions_validator builds each key with ".".join(part.this for part in v.this.parts). part.this is the bare identifier string, so the quoted flag is gone before normalize_identifiers runs, and the key is normalized as if it had never been quoted. On a dialect where quoting makes a column case-sensitive, "myColumn" becomes MYCOLUMN, matches no column, and gets dropped along with the rest of the table's comments.

Normalizing each part while it is still an identifier fixes it. Unquoted keys normalize exactly as before, and the dict branch used by Python models is unchanged in behaviour.

Before, with dialect snowflake:

descriptions: {'MYCOLUMN': 'comment for a case-sensitive column'}
columns     : ['myColumn']

After: {'myColumn': ...}.

test_column_descriptions_quoted_identifier covers both halves, a quoted key keeping its case and an unquoted one still normalizing, and fails on main with MYCOLUMN != myColumn.

tests/core/test_model.py passes in full (334). ruff and ruff-format pass. mypy on the changed module reports nothing.

_column_descriptions_validator built each key with part.this, which is the
bare identifier string, so the quoted flag was gone before
normalize_identifiers ran. A quoted key was then normalized as if it were
unquoted, and on dialects where quoting makes a column case-sensitive the
resulting name matched no column, so the description was dropped along with
the rest of the table's comments.

Normalize each part while it is still an identifier. Unquoted keys normalize
exactly as before.

Fixes SQLMesh#5943

Signed-off-by: ReguiguiMohamed <mohamedreguigui2004@gmail.com>
@StuffbyYuki

Copy link
Copy Markdown
Collaborator

@ReguiguiMohamed Thanks for this PR!

Any thoughts on this? Adding a dotted-key case (nested.field / "MyStruct"."myField"). That join-by-parts path is what changed, and unquoted dotted names will now normalize per part (NESTED.FIELD on Snowflake) instead of as one identifier.

Not a blocker though

The join-by-parts path had no test. A BigQuery nested path normalizes the
same as before the fix, since joining the parts and re-parsing produced a
quoted identifier that BigQuery lowercases anyway. On Snowflake an unquoted
path now normalizes per part, which is what an unquoted name should do
there, and a quoted one keeps its case.

Signed-off-by: ReguiguiMohamed <mohamedreguigui2004@gmail.com>
@ReguiguiMohamed

Copy link
Copy Markdown
Author

@StuffbyYuki Thanks for the review, and I'm glad to be contributing here.

Good catch. I ran both cases against the pre-fix code.

BigQuery is unchanged. A nested path still normalizes to record.myfield. Joining the parts produces a string with a dot in it, to_identifier marks that quoted, and BigQuery lowercases quoted identifiers too, so the key was already fully normalized before. The nested-field path added in #3887 keeps working.

Snowflake does change. nested.field used to stay nested.field and is now NESTED.FIELD. Same quoting artifact, but Snowflake leaves quoted identifiers alone, so the old key skipped normalization entirely. An unquoted name should uppercase there like a plain field does, so I kept the new behavior. "MyStruct"."myField" comes out the same either way.

Test for both pushed in 6df27fe.

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.

column_descriptions drops identifier quoting, breaking all column comments on case-sensitive columns

2 participants