Skip to content

DM-51789: Add support for looking up column references by name#189

Open
JeremyMcCormick wants to merge 2 commits into
mainfrom
tickets/DM-51789
Open

DM-51789: Add support for looking up column references by name#189
JeremyMcCormick wants to merge 2 commits into
mainfrom
tickets/DM-51789

Conversation

@JeremyMcCormick

@JeremyMcCormick JeremyMcCormick commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

This updates Felis to support referencing of columns by their name instead of their ID, described in RFC-1111. The ID lookup is also supported for backward compatibility, at least for the deprecation period.

The primaryKey field still uses the ID for now. This will be resolved on a separate PR for DM-46055.

Checklist

  • Ran Jenkins
  • Added a release note for user-visible changes to docs/changes

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.11538% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.01%. Comparing base (610ed3f) to head (f49cb6e).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
python/felis/datamodel.py 97.14% 1 Missing and 1 partial ⚠️
python/felis/metadata.py 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   91.89%   92.01%   +0.12%     
==========================================
  Files          13       13              
  Lines        2060     2116      +56     
  Branches      299      313      +14     
==========================================
+ Hits         1893     1947      +54     
- Misses        109      110       +1     
- Partials       58       59       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Copilot AI 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.

Pull request overview

Adds name-based resolution of column references across the Felis datamodel and downstream consumers, while preserving backward compatibility with legacy global column IDs.

Changes:

  • Introduces name-first column resolution (Table._find_column) and updates schema validation for constraints/indexes.
  • Adds ForeignKeyReference to support a name-based foreign key target (reference) alongside legacy referencedColumns.
  • Updates SQLAlchemy metadata builder, TAP_SCHEMA loader, and test fixtures to use name-based references.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/test_metadata.py Updates metadata assertions to resolve constraint/index column refs via name-first lookup.
tests/test_datamodel.py Adds tests covering name-based lookup for column groups, indexes, and foreign keys (including reference).
tests/data/sales.yaml Migrates fixture constraints to name-based column refs and adds new constraints using reference.
python/felis/tap_schema.py Resolves foreign keys using name-based refs (and legacy fallback) when generating TAP_SCHEMA keys/key_columns rows.
python/felis/metadata.py Switches metadata building to resolve columns by name-first lookup and maps Felis objects to SQLAlchemy objects by identity.
python/felis/datamodel.py Adds ForeignKeyReference, name-first column lookup, FK exclusivity checks, and schema validation for index/constraint refs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread python/felis/datamodel.py Outdated
Comment thread python/felis/metadata.py Outdated
Comment thread python/felis/metadata.py Outdated
Comment thread python/felis/datamodel.py
Comment thread python/felis/datamodel.py Outdated
This partially implements the triggered changes from RFC-1111. The
column references in constraints and indexes now support either a name
in the current table or an ID for backward compatibility.

The `primaryKey` field was left alone for now and will be updated to
support names on a separate ticket.
@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review July 8, 2026 19:24
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.

2 participants