Skip to content

fix(data-warehouse): match the id primary-key fallback case-insensitively - #71064

Merged
Gilbert09 merged 1 commit into
masterfrom
tom/dwh-case-insensitive-id-pk-fallback
Jul 15, 2026
Merged

fix(data-warehouse): match the id primary-key fallback case-insensitively#71064
Gilbert09 merged 1 commit into
masterfrom
tom/dwh-case-insensitive-id-pk-fallback

Conversation

@Gilbert09

Copy link
Copy Markdown
Member

Problem

A Snowflake import with a table that has an ID column failed incremental syncs with "Primary key required for incremental syncs", and the UI didn't preselect the primary key at setup.

Snowflake uppercases unquoted identifiers, so the column surfaces as ID everywhere. SHOW PRIMARY KEYS only returns declared PK constraints — which Snowflake doesn't enforce and most tables don't have — so detection falls through to the id-column name fallback. That fallback compared against the literal lowercase string "id" in all three places it exists (SQL-source discovery ×2 and the sync-time resolve_primary_keys last resort), so ID missed every one, no key was resolved, and the keyless-table guardrail fired. This affects any Snowflake table without a declared PK constraint, and any other source reporting uppercase column names.

Changes

All three fallbacks now match the id column case-insensitively and return the column's actual stored casing (["ID"], not a hardcoded ["id"]):

  • sources/common/sql/types.pyresolve_detected_primary_keys (Postgres / MySQL / MSSQL / Redshift discovery)
  • sources/common/sql/base.pySQLSource._default_primary_key_from_columns (generic SQL discovery)
  • pipelines/common/extract.pyresolve_primary_keys (sync-time last resort)

Returning the actual casing matters: the merge indexes extracted batches by the real column name, so returning a lowercase "id" for an ID column would just move the same failure into the merge.

How did you test this code?

Automated tests I (Claude) ran locally, all green:

  • New test_types.py for resolve_detected_primary_keys (parameterized: discovered-wins, lowercase fallback, uppercase ID returning actual casing, no-id → None) — no test covered this helper at all; the uppercase case is the production regression.
  • One new case in the existing TestResolvePrimaryKeys precedence table: {"name": "ID"} resolves to ["ID"] — catches both a revert to case-sensitive matching and a change to returning hardcoded lowercase.
  • Full test_extract.py + sources/common/sql/tests/ suites: 365 passed.

_default_primary_key_from_columns carries the identical three-line change and is exercised transitively through the SQL-source discovery suites; a third copy of the same parameterized matrix wouldn't catch anything new.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

No behavior documented externally changes — PK detection just works for uppercase ID columns now. Affected users can also self-serve today by setting the primary key manually in the table's sync settings (user overrides take precedence).

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Claude Code session, directed by Tom, from a user report of a Snowflake ID (type text) column not being detected. Investigation confirmed the column type is irrelevant (the fallback only looks at names) and the uppercase identifier is the whole story. Skill invoked: /writing-tests.

@Gilbert09 Gilbert09 self-assigned this Jul 15, 2026
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team July 15, 2026 13:07
@trunk-io

trunk-io Bot commented Jul 15, 2026

Copy link
Copy Markdown

Static BadgeStatic BadgeStatic BadgeStatic Badge

View Full Report ↗︎Docs

@Gilbert09 Gilbert09 added the stamphog Request AI approval (no full review) label Jul 15, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small, well-tested, contained fix to a case-sensitivity bug in primary-key fallback logic; diff matches description, author is on the owning team, and no risky territory (no schema/API/auth/billing/CI surface) is touched.

  • Author wrote 0% of the modified lines and has 8 merged PRs in these paths (familiarity MODERATE).
  • 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 31L, 3F substantive, 56L/5F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1b-small (56L, 5F, single-area, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ c8f67df · reviewed head c8e8d9f

@Gilbert09
Gilbert09 merged commit 44daa06 into master Jul 15, 2026
356 checks passed
@Gilbert09
Gilbert09 deleted the tom/dwh-case-insensitive-id-pk-fallback branch July 15, 2026 15:53
@deployment-status-posthog

deployment-status-posthog Bot commented Jul 15, 2026

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-07-15 16:31 UTC Run
prod-us ✅ Deployed 2026-07-15 16:51 UTC Run
prod-eu ✅ Deployed 2026-07-15 16:53 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant