Skip to content

Fixes #28077: [SNOWFLAKE INGESTION METADATA] Duplicate column case - #32500

Merged
TeddyCr merged 1 commit into
mainfrom
agent/issue-28077
Sep 4, 2026
Merged

Fixes #28077: [SNOWFLAKE INGESTION METADATA] Duplicate column case#32500
TeddyCr merged 1 commit into
mainfrom
agent/issue-28077

Conversation

@TeddyCr

@TeddyCr TeddyCr commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

This pull request was opened by an olivaw agent. It is an input to review, not an approved change — read the diff, not this description.

Fixes #28077

What olivaw checked (machine-observed, §8.2/A.1)

  • diff: non-empty
  • build: not_run
  • tests: not_run
  • base: 77cae1608394c14e81054e8f6aab9c2a3e1eb999
  • head: 66827ec5ef5c10a355dc3290cab501d1add4e33b

No build or test command is configured for this workspace, so only the diff's existence was checked. Set run_config.build_cmd/test_cmd to have olivaw verify its own work.

What the agent says it did (unverified)

Committed on agent/issue-28077.

Root cause

ometa_to_sqa_orm builds a declarative SQLAlchemy class from the OM table entity, and build_orm_col set every column's SQA key to the lowercased name (ingestion/src/metadata/profiler/orm/converter/base.py:98). SQLAlchemy indexes a table's column collection by key, so a Snowflake table containing both "Hotel_region" and HOTEL_REGION produced two columns with the key hotel_region — the second one evicted the first from __table__.c, and mapper configuration then blew up with exactly the error in the screenshot:

When configuring property 'Hotel_region' on ... column '<table>.Hotel_region' is not
represented in the mapper's table. Use the `column_property()` function ...

Because the ORM class is built for the whole table, this killed any test on that table, not just tests on the duplicated column — matching the report.

Fix

build_orm_col_keys computes the keys for all columns at once: a column keeps the lowercase key only when its lowercase name is unambiguous; when two columns collide case-insensitively, each keeps its original (already unique) name as the key. build_orm_col takes an optional key and otherwise behaves as before, so the existing callers (Athena, Databricks, tableDiff) are untouched.

Verification

The repo has no Python env, so I generated the pydantic models (scripts/datamodel_generation.py + make py_antlr), installed the deps needed by the converter, and stubbed the unbuildable data_diff transitive dep.

  • New test test_columns_differing_only_in_case in ingestion/tests/unit/observability/profiler/test_converter.py fails on the pre-fix code with the exact ArgumentError above and passes with the fix.
  • tests/unit/observability/profiler/test_converter.py: 10 passed.
  • Wider tests/unit/observability/profiler + tests/unit/sampler: 533 passed, 6 failed — all six are ModuleNotFoundError for optional connector packages (bigquery, teradata driver, confluent_kafka), unrelated to this change; the remaining collection errors are the same missing-dependency kind.
  • ruff check / ruff format clean.

Snowflake allows a table to hold columns whose names only differ in their
casing (e.g. "Hotel_region" and HOTEL_REGION). The ORM converter keys every
SQA column by its lowercase name, so those columns collided in the table's
column collection: one of them was dropped and the declarative mapper failed
with "column <table>.<column> is not represented in the mapper's table",
aborting the whole profiler/data quality workflow.

Keep the original name as the SQA key when the lowercase name is ambiguous,
so every column is mapped and can be profiled or tested.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 2, 2026 19:23
@TeddyCr
TeddyCr requested a review from a team as a code owner September 2, 2026 19:23
@github-actions github-actions Bot added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Sep 2, 2026
@gitar-bot

gitar-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Fixes duplicate column case sensitivity issue in Snowflake ingestion by preserving original column names as keys when they differ only in casing. The fix updates build_orm_col to accept an optional key parameter, computed by build_orm_col_keys to handle case-insensitive collisions while keeping existing callers unchanged. New test verifies the fix resolves the mapper configuration error. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix directly addresses the reported mapper failure mode and includes a targeted regression unit test that exercises the corrected behavior.

Pull request overview

This PR fixes Snowflake ingestion/profiler failures when a table contains two columns that differ only by casing (e.g., "Hotel_region" and HOTEL_REGION), which previously collided due to lowercased SQLAlchemy column keys and caused ORM mapper configuration errors.

Changes:

  • Add build_orm_col_keys() to compute per-column SQLAlchemy key values that remain unique when case-insensitive collisions occur.
  • Update ometa_to_sqa_orm() to pass the computed per-column key into build_orm_col(), preventing column eviction from SQLAlchemy’s column collection.
  • Add a unit regression test covering the case-only-different duplicate column scenario for Snowflake.
File summaries
File Description
ingestion/src/metadata/profiler/orm/converter/base.py Computes unique SQLAlchemy column keys to avoid case-insensitive collisions and wires them into ORM generation.
ingestion/tests/unit/observability/profiler/test_converter.py Adds a regression unit test ensuring both case-variant columns are preserved and keyed safely.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@sonarqubecloud

sonarqubecloud Bot commented Sep 2, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 66827ec5ef5c10a355dc3290cab501d1add4e33b in Playwright run 33672971880, attempt 1.

✅ 109 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 53m 47s

⏱️ Max setup 6m 33s · max shard execution 13m 3s · max shard-job elapsed before upload 20m 5s · reporting 5s

🌐 219.35 requests/attempt · 1.78 app boots/UI scenario · 0.00% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 219.35 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 1.78 per UI scenario (217 boots / 122 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 46 0 0 0 0 0
✅ Shard ingestion-01 29 0 0 0 0 0
🟡 Shard ingestion-02 34 0 1 0 0 0
🟡 1 flaky test(s) (passed on retry)
  • Features/DataQuality/DataQuality.spec.tsPagination functionality in test cases list (shard ingestion-02, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@TeddyCr
TeddyCr enabled auto-merge September 3, 2026 19:07
)


def build_orm_col_keys(columns: list[Column]) -> list[str]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_orm_col still defaults to name.lower() when no key= is passed, and the Athena/Databricks profiler interfaces rebuild the columns after ometa_to_sqa_orm without it (athena/profiler_interface.py:106, databricks/profiler_interface.py:146) — __table__.c drops back to 2 keys there and Hotel_region gets evicted. Moving the key computation into a shared build_orm_cols(columns, service_type) would stop callers forgetting it.

quote=quote,
key=str(col.name.root).lower(), # Add lowercase column name as key for snowflake case sensitive columns
# Add lowercase column name as key for snowflake case sensitive columns
key=key or str(col.name.root).lower(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
key=key or str(col.name.root).lower(),
key=key if key is not None else str(col.name.root).lower(),

Also, build_orm_col_keys could use a direct test (e.g. ["Foo", "FOO", "foo"]).

@TeddyCr
TeddyCr added this pull request to the merge queue Sep 3, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — failed_checks (2026-09-04T19:02:53Z)

Blocked the queue: playwright-summary

@TeddyCr
TeddyCr merged commit 5f62431 into main Sep 4, 2026
105 checks passed
@TeddyCr
TeddyCr deleted the agent/issue-28077 branch September 4, 2026 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SNOWFLAKE INGESTION METADATA] Duplicate column case

3 participants