Skip to content

DM-51789: Support column reference lookup by name in schema_model - #148

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

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

Conversation

@JeremyMcCormick

@JeremyMcCormick JeremyMcCormick commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR updates python/lsst/dax/apdb/schema_model.py to support column referencing using names, while preserving backward compatibility with ID lookup. Details and motivation for this change can be found in RFC-1111. The Felis models which were affected include constraints, in particular foreign keys, and indexes, as well as the primaryKey field.

New (name-based) example:

constraints:
- name: fk_diaSource_diaObject
  "@type": ForeignKey
  columns: ["diaObjectId"]
  reference:
    table: DiaObject
    columns: ["diaObjectId"]

Legacy (ID-based) example:

constraints:
- name: fk_diaSource_diaObject
  "@type": ForeignKey
  columns: ["#DiaSource.diaObjectId"]
  referencedColumns: ["#DiaObject.diaObjectId"]

Minimal changes were made to the existing conversion logic to support both lookup schemes in a backward-compatible way. Schemas with either the new, name-based referencing should be processed correctly, as well as those using IDs.

Implementation Details

1) Introduced a small lookup object with two explicit maps

A lightweight _ColumnLookup helper was added to hold both lookup strategies:

  • columns_by_id: Mapping[str, Column]
  • columns_by_table_name: Mapping[tuple[str, str], Column]

find_table_column(table_name, column_ref) performs name-first lookup with ID fallback:

  1. try (table_name, column_ref) in columns_by_table_name
  2. fallback to column_ref in columns_by_id

This allows both formats to be supported simultaneously with a single lookup call.

2) Updated conversion call paths to use one lookup reference

Rather than receiving raw dictionaries, conversion methods now use one lookup object:

  • Index.from_felis(..., dm_table, lookup)
  • Constraint.from_felis(..., dm_table, lookup)
  • Table.from_felis(..., lookup)

This keeps the method signatures simple with a single object for accessing the referenced columns.

3) Foreign key handling supports both encodings

In Constraint.from_felis for ForeignKeyConstraint:

  • Source columns are resolved table-locally via find_table_column.
  • If dm_constr.reference is present, referenced columns are resolved by (reference.table, column_name) from columns_by_table_name.
  • Otherwise (legacy path), referenced columns are resolved from dm_constr.referenced_columns via columns_by_id.

This ensures that the new and legacy styles are both supported.

4) Map construction in Schema.from_felis

During schema conversion, all converted columns are indexed into both maps before table conversion:

  • ID map enables legacy compatibility.
  • (table, name) map enables the new name-based lookup and avoids ambiguous global name matching.

Tables are then built from Table.from_felis(dm_table, lookup).

Tests

A new module was added: tests/test_schema_model.py.

It tests the two referencing strategies as well as conversion invariants, including:

  1. test_index_column_lookup_by_name: index definitions that reference columns by name are resolved to the correct converted column objects.
  2. test_foreign_key_legacy_referenced_columns: legacy foreign keys that use referencedColumns are still resolved correctly, preserving backward compatibility.
  3. test_foreign_key_reference_style: new foreign keys that use reference.table and reference.columns are resolved correctly.
  4. test_column_table_backrefs: every converted column keeps a back-reference to the table that owns it.
  5. test_foreign_key_referenced_column_identity: referenced foreign key columns point to the exact column objects from the parent table (not duplicate objects).
  6. test_foreign_key_multicolumn_order_preserved: multi-column foreign keys preserve the source and referenced column ordering from the schema definition.
  7. test_foreign_key_styles_equivalent: legacy and new foreign key encodings produce equivalent converted foreign key structures.
  8. test_foreign_key_missing_referenced_table_raises and test_foreign_key_missing_referenced_column_raises: invalid referenced table/column definitions are rejected at the expected validation layer.
  9. test_schema_model_conversion: validate conversion from Felis to all target schema model classes (not strictly in scope for this ticket but useful for full change validation).

There was previously no dedicated test case for schema_model.py, so this should be useful for verifying changes in the future.

Notes

The temporary lsst-felis branch pin in requirements.txt is for development and will be replaced by main before merging.

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.

Pull request overview

Updates dax_apdb’s internal schema-model conversion to correctly resolve Felis column references (including name-based references) by switching to a two-pass table/column construction and resolving PK/index/constraint references after all tables exist.

Changes:

  • Switch Schema.from_felis() to a two-pass conversion to enable cross-table foreign key resolution without a global ID→Column map.
  • Update index/constraint/table conversion to resolve referenced columns via Felis table/schema lookup APIs.
  • Point lsst-felis dependency at the tickets/DM-51789 branch needed for the updated reference behavior.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.

File Description
requirements.txt Uses the tickets/DM-51789 branch of lsst-felis to pick up needed schema-reference behavior.
python/lsst/dax/apdb/schema_model.py Refactors Felis→internal schema conversion to resolve column references via table/schema lookups and a two-pass build.

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

Comment thread python/lsst/dax/apdb/schema_model.py Outdated
Comment thread python/lsst/dax/apdb/schema_model.py Outdated
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.23%. Comparing base (b4d6f81) to head (777b59b).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   84.90%   85.23%   +0.32%     
==========================================
  Files          73       74       +1     
  Lines        7408     7536     +128     
  Branches      850      855       +5     
==========================================
+ Hits         6290     6423     +133     
+ Misses        881      875       -6     
- Partials      237      238       +1     

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

@JeremyMcCormick
JeremyMcCormick force-pushed the tickets/DM-51789 branch 4 times, most recently from 1e1d9c4 to c995350 Compare July 30, 2026 02:39
@JeremyMcCormick JeremyMcCormick changed the title DM-51789: Update construction of internal schema model to support column name refs DM-51789: Update construction of internal schema model to support column name referencing Jul 30, 2026
@JeremyMcCormick JeremyMcCormick changed the title DM-51789: Update construction of internal schema model to support column name referencing DM-51789: Support Column Reference Lookup by Name in schema_model Jul 30, 2026
@JeremyMcCormick JeremyMcCormick changed the title DM-51789: Support Column Reference Lookup by Name in schema_model DM-51789: Support column reference lookup by name in schema_model Jul 30, 2026
@JeremyMcCormick
JeremyMcCormick requested a review from Copilot July 30, 2026 02:54

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.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

python/lsst/dax/apdb/schema_model.py:330

  • Docstring parameter name does not match the function signature (dm_const vs dm_constr), which can confuse readers and generated docs.
        dm_const : `felis.datamodel.Constraint`

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.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

python/lsst/dax/apdb/schema_model.py:372

  • For ForeignKeyConstraint conversion, the Felis reference field (new style) isn’t stripped from annotations, so name-based schemas may unexpectedly carry the full reference object in ForeignKeyConstraint.annotations (legacy schemas don’t). Add "reference" to the stripped keys to keep converted output consistent across both encodings.
                referenced_columns=referenced_columns,
                deferrable=dm_constr.deferrable,
                initially=dm_constr.initially,
                description=dm_constr.description,

@JeremyMcCormick
JeremyMcCormick force-pushed the tickets/DM-51789 branch 3 times, most recently from 67d1e22 to 6a3e81d Compare July 30, 2026 19:22
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