Repository navigation
fix(schema_loader): quote case-sensitive column names in external models - #6099
ReguiguiMohamed wants to merge 1 commit into
Conversation
create_external_models wrote column names bare, so a Postgres column "ID" was read back as id. A name is now quoted only when the loader's identifier normalization would change it. Names that already round-trip are written as before. Before the fix the new test failed with '"ID"' != 'ID' on Postgres and '"lower_case"' != 'lower_case' on Snowflake. Fixes SQLMesh#6058 Signed-off-by: ReguiguiMohamed <mohamedreguigui2004@gmail.com>
0f21647 to
ae50f5b
Compare
|
Rebased on main, since the CI runs expired before they were approved. @StuffbyYuki, could you approve the workflows and take a look when you have time? |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Serialize and validate column names sqlmesh/core/schema_loader.py, tests/core/test_schema_loader.py |
get_columns compares each name with its dialect-normalized identifier and explicitly identifies names that differ. Tests verify generated YAML keys and confirm that loading restores the original names across three dialects. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: benfdking
Merge Risk: 
ae50f
-
No merge-blocking issue was identified in the column-name preservation change.
Security architecture risk: ae50f
-
The change preserves column-name spelling without adding an execution entrypoint, increasing permissions, or expanding which tables are inspected. The reviewed generation and loading flow shows no material security risk introduced by this PR.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The introduced effect is bounded to column keys in generated external-model schemas and their subsequent interpretation. The inspected change does not expand table selection, connection authority, or output-path selection.
Trust Boundaries and Controls
- observed — Column names returned by the adapter cross into persisted schema metadata. Normalization-sensitive names are now explicitly identified before serialization, and the added tests check that reload preserves their identity rather than changing case.
Resilience and Maintainability Implications
- observed — Normalization runs inside the existing exception boundary. Strict discovery failures raise before the destination is opened; non-strict failures warn and omit the failed schema. The final file overwrite remains non-atomic, and no invocation-level locking is added. Those writer behaviors predate this PR rather than constituting newly introduced concerns.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the primary change: quoting case-sensitive column names in external models. Description check The description follows the required template. It explains the change, lists the test plan and results, and completes all checklist items. Linked Issues check Issue #6058requires that create_external_models quote column names whose unquoted form the target dialect would change, and leave other names unquoted, so generated keys round-trip through the loader…Out of Scope Changes check The whole-PR changes are limited to sqlmesh/core/schema_loader.py (+7/-1: two imports and the get_columns loop) and tests/core/test_schema_loader.py (+46 lines: one parametrized test). The imports and… Full details: Linked Issues check
-
Explanation
Issue
#6058requires that create_external_models quote column names whose unquoted form the target dialect would change, and leave other names unquoted, so generated keys round-trip through the loader. At head, sqlmesh/core/schema_loader.py get_columns (diff against merge base) calls normalize_identifiers(c, dialect=dialect).name for each column. If the result differs from c, it serializes c with exp.to_identifier(c).sql(dialect=dialect, identify=True). Column types are still serialized with dtype.sql(dialect=dialect). This yields '"ID"' for Postgres uppercase names and leaves lowercase and Snowflake-canonical names unchanged, matching the expected output in#6058. The test test_create_external_models_quotes_case_sensitive_columns (readable at head in tests/core/test_schema_loader.py) is parametrized for postgres, snowflake, and clickhouse. It asserts the written YAML keys and that create_external_model restores the original column names. This covers the automated-test requirement. The implementation matches the issue's expected behavior. I did not run the tests. Test results rely on the PR description and were not independently verified. The issue's optional early return for already-quoted names is not implemented. The adapter returns raw names in this code path, so the omission does not leave an unmet requirement.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR changes are limited to sqlmesh/core/schema_loader.py (+7/-1: two imports and the get_columns loop) and tests/core/test_schema_loader.py (+46 lines: one parametrized test). The imports and the loop directly implement the column-name quoting required by
#6058. The test covers the same change. No unrelated files or behavior changes appear in the reviewed diff. The type serialization and strict/warning error handling are unchanged. The changed-blob diff tool was unavailable for the test file, so the test diff was read by file content at head and its line count comes from the supplied summary.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Figured I would add some context, since this fix overlaps with work I tried earlier. I opened a very similar change for quoting case-sensitive column names in external models on 12 September 2026 (#6059). It got no maintainer response, so I sent a polite check-in on 21 September 2026, and the thread stayed silent. My automated dormancy workflow closes pull requests that stay inactive that long, so it closed #6059 on 23 September 2026 at 19:36 UTC. I am glad to see this one up and hope it gets the attention it deserves. |
Description
Preserve column names when generating external models. PostgreSQL
"ID"was loaded asid.Builds on @devtechedge's closed PR #6059, using SQLGlot identifier serialization and adding Snowflake and ClickHouse coverage.
Fixes #6058
Test Plan
test_create_external_models_quotes_case_sensitive_columnscovers Postgres, Snowflake and ClickHouse. It checks the written keys and the round trip throughcreate_external_model.pytest tests/core/test_schema_loader.py: 11 passedpytest tests/core/test_model.py -k external: 4 passedpytest tests/core/linter/test_builtin.py tests/lsp tests/core/test_schema_loader.py -m "not slow and not docker": 76 passedChecklist
make styleand fixed any issues (ranmake py-stylein WSL, covering every configured hook)make fast-test)git commit -s) per the DCOSummary by CodeRabbit