DM-53310: Implement RFC-1138 changes (solar system tables) #424

mjuric · 2025-11-22T01:25:51Z

Checklist

When making changes to YAML files in the schemas directory:

If applicable, incremented the schema version number, following the guidelines in the contribution guide
Referred to the documentation on specific schemas for additional versioning information, change constraints, or tasks that may need to be performed, based on which schema is being updated
Ran Jenkins
Added a news fragment describing the changes

andy-slac · 2025-11-24T02:58:33Z

python/lsst/sdm/schemas/apdb.yaml

Could you rearrange your changes so that diff shows actual differences in SSObject/SSSource tables? With current arrangement it's hard to guess which columns were added or dropped.

The issue is that the part of the change is the reordering of the fields themselves, so they show up in a logical order.

If it helps, sdm-schemas.lsst.io sorts alphabetically so maybe that's a way to compare for differences (here's before and after)?

Comparing web pages by eye is hard. We have felis diff, but it is not super user-friendly. @JeremyMcCormick, could we extend felis diff to produce a human-readable summary that ignores table and column re-ordering?

JeremyMcCormick · 2025-11-24T17:25:09Z

@mjuric

There should be no need to prepend commit messages with the name of the schema like lsstcam.yaml: as the files affected are available in the log. We haven't generally done this in the repo.

Also, I think before merging this can be rebased down to a single commit or maybe just a few.

Finally, commit messages should start with an implicit verb like "Fix," "Change," etc.

:)

JeremyMcCormick · 2025-11-24T17:31:08Z

This check failure can be ignored:

https://github.com/lsst/sdm_schemas/actions/runs/19615135537/job/56166612401?pr=424

The tool is erroneously identifying one of your columns as a "band column."

The band column checker should have a switch for ignoring specific columns - I will make a ticket for this.

mjuric · 2025-11-24T19:07:34Z

@mjuric

There should be no need to prepend commit messages with the name of the schema like lsstcam.yaml: as the files affected are available in the log. We haven't generally done this in the repo.

Also, I think before merging this can be rebased down to a single commit or maybe just a few.

Finally, commit messages should start with an implicit verb like "Fix," "Change," etc.

:)

All makes sense -- will do!

Right now what you're seeing is a lot of development in progress (may be good to ignore it); I'll squash/rebase at the end into a sensible set of commits.

mjuric · 2025-11-25T16:30:03Z

Cleaned up the commits down to three logically distinct ones (though the PR is still showing the entire messy history), rebased to latest main, ready for review & merge.

JeremyMcCormick · 2025-11-25T16:31:44Z

Minor suggestion: changelog update commit can instead be "Add news fragment".

JeremyMcCormick · 2025-11-25T17:01:05Z

@andy-slac The ids in apdb.yaml are removed in this PR. If
accepted, the dax_apdb code will need to pass "id_generation": True context to validate the schema now when loading the model. You can do that with model_validate or (preferably) Schema.from_uri. This will make Felis generate the ids automatically when loading in the schema.

You may want to do that immediately before this is even merged as merging this first could result in a broken weekly release. It is harmless to turn that on if the ids are already there as only missing ones are generated.

andy-slac

I only looked at apdb.yaml, it looks OK with few questions/comment. As this patch does not really touch anything in APDB tables, we do not need to change version number.

andy-slac · 2025-11-25T17:23:43Z

python/lsst/sdm/schemas/apdb.yaml

 name: "ApdbSchema"
-"@id": "#apdbSchema"
-version: "9.1.0"
+version: "10.0.0"


I do not think we need a new version number, APDB database does not care about Solar System tables, and recent dax_apdb update dropped the only table that existed in APDB. We do not want to force people into doing unnecessary schema upgrades. Once we have sso.yaml we may want to track version changes in that schema, but that depends also on the clients of that schema.

andy-slac · 2025-11-25T17:35:56Z

python/lsst/sdm/schemas/apdb.yaml

+# FIXME: commented out as it generates a
+# >           raise ValueError(f"Dependency cycle in foreign keys: {tables}")
+# exception. Unclear why, but as these aren't used in APDB yet there should be no harm. See:
+#    https://rubin-obs.slack.com/archives/C07QJMPRMLJ/p1763919304828269?thread_ts=1763826894.543509&cid=C07QJMPRMLJ
+# for some discussion.


I think you can uncomment this already, latest dax_apdb should be fixed.

andy-slac · 2025-11-25T17:38:23Z

python/lsst/sdm/schemas/apdb.yaml

+  - name: idx_SSObject_ssObjectId
+    description: Unique index on the ssObjectId column
+    columns:
+    - "#SSObject.ssObjectId"


ssObjectId is already a primary key, no need for an additional index.

andy-slac · 2025-11-25T17:45:34Z

python/lsst/sdm/schemas/apdb.yaml

+  - name: idx_SSObject_designation
+    description: >-
+      Unique index on the designation column
+    columns:
+    - "#SSObject.designation"


If you want a unique index then you need to define a constraint, not an index, e.g.:

constraints: - name: unique_SSObject_designation "@type": Unique description: ... columns: ["#SSObject.designation"]

andy-slac · 2025-11-25T17:53:59Z

python/lsst/sdm/schemas/apdb.yaml

+      closeness to the predicted SSO position.  If diaSourceId is the
+      nearest DiaSource to this SSO prediction, diaSourceDistanceRank=1
+      would be set. If it is the second nearest, it would be 2, etc.
+    datatype: int


Could be short?

andy-slac · 2025-11-25T18:17:26Z

python/lsst/sdm/schemas/apdb.yaml

+    description: This is a set of unique identifier_ids in an array that points to the identification_metadata
+      table.


What is identification_metadata? What is the encoding for the array (JSON)?

We replicate this table from the MPC, and keep the exact schema (and docs) as upstream. This refers to a different table at the MPC, that's not relevant for the queries we're hoping to support with the replica here.

We are exposing description is schema browser, people may get confused if you mention something that does not exist.

andy-slac · 2025-11-25T18:19:26Z

python/lsst/sdm/schemas/apdb.yaml

+    nullable: false
+    ivoa:ucd: meta.id;src
+  - name: object_type
+    description: Integer to indicate the object type. To be linked (foreign key) to object_type lookup table


There is no object_type table in this schema.

This is also a replica from the MPC (including the doc strings). We didn't import object_type (yet) as we don't need it.

andy-slac · 2025-11-25T18:20:58Z

python/lsst/sdm/schemas/apdb.yaml

+  - name: idx_current_identifications_packed_primary_provisional_desig
+    description: Unique index on the packed_primary_provisional_designation column
+    columns:
+    - '#current_identifications.packed_primary_provisional_designation'
+  - name: idx_current_identifications_packed_secondary_provisional_desig
+    description: Unique index on the packed_secondary_provisional_designation column
+    columns:
+    - '#current_identifications.packed_secondary_provisional_designation'


Unique indices need to be in constraints:. Is each of these columns unique or only their combination?

andy-slac · 2025-11-25T18:24:18Z

python/lsst/sdm/schemas/apdb.yaml

+    ivoa:ucd: time.processing;meta.dataset
+  indexes:
+  - name: idx_numbered_identifications_iau_name
+    description: Unique index on the iau_name column


Same - all unique indices should be constraints.

andy-slac · 2025-11-25T18:26:30Z

python/lsst/sdm/schemas/apdb.yaml

+- name: SSSource
+  description: LSST-computed per-source quantities. 1:1 relationship with DiaSource.
+  tap:table_index: 120
+  primaryKey: '#SSSource.diaSourceId'


I think we prefer double colons everywhere for consistency.

The double colons -- you mean as "1::1" or elsewhere?

Sorry, I'm typing without reading - I mean double quotes 🙂

Still need to convert to double quotes here

andy-slac · 2025-11-25T18:59:57Z

the dax_apdb code will need to pass "id_generation": True context

This is already on main.

mjuric · 2025-11-25T19:46:54Z

Minor suggestion: changelog update commit can instead be "Add news fragment".

👍 -- updated to "Add news fragment for RFC-1138 (SS* table) changes".

mjuric · 2025-11-25T20:04:10Z

@andy-slac @JeremyMcCormick I think I've implemented all the review comments (thanks!). I also synced up lsstcam.yaml with comments on apdb.yaml that transfer over.

Ready to be green-lit for merging?

andy-slac · 2025-11-25T20:45:14Z

Ready to be green-lit for merging?

Approved!

isullivan

Some minor comments on formatting.

isullivan · 2025-12-02T04:28:51Z

python/lsst/sdm/schemas/apdb.yaml

+    - '#SSObject.designation'
+    referencedColumns:
+    - '#mpc_orbits.designation'


Double quotes?

isullivan · 2025-12-02T04:29:48Z

python/lsst/sdm/schemas/apdb.yaml

+- name: SSSource
+  description: LSST-computed per-source quantities. 1:1 relationship with DiaSource.
+  tap:table_index: 120
+  primaryKey: '#SSSource.diaSourceId'


Still need to convert to double quotes here

isullivan · 2025-12-02T04:32:32Z

python/lsst/sdm/schemas/apdb.yaml

+    description: Link an SSSource to its associated DiaSource
+    '@type': ForeignKey
+    columns:
+    - '#SSSource.diaSourceId'


Here and in the following ~20 lines, use double quotes for quotes in columns

isullivan · 2025-12-02T04:34:09Z

python/lsst/sdm/schemas/apdb.yaml

+    description: Uniqueness of unpacked_primary_provisional_designation
+    '@type': Unique
+    columns:
+    - '#mpc_orbits.unpacked_primary_provisional_designation'


Here and the next couple columns, use double quotes

isullivan · 2025-12-02T04:35:23Z

python/lsst/sdm/schemas/apdb.yaml

+    "@type": Unique
+    description: Unique index on the packed_primary_provisional_designation column
+    columns:
+    - '#current_identifications.packed_primary_provisional_designation'


Here and next column, double quotes

@id

Also drop "@id" columns which can now be auto-generated.

mjuric marked this pull request as draft November 22, 2025 01:26

mjuric force-pushed the tickets/DM-53310 branch 4 times, most recently from 617363b to 0fee2c2 Compare November 23, 2025 01:55

andy-slac reviewed Nov 24, 2025

View reviewed changes

mjuric force-pushed the tickets/DM-53310 branch 4 times, most recently from e8a0a5d to a41fdc8 Compare November 25, 2025 16:24

mjuric marked this pull request as ready for review November 25, 2025 16:28

andy-slac reviewed Nov 25, 2025

View reviewed changes

mjuric force-pushed the tickets/DM-53310 branch 2 times, most recently from 878b4ec to 185ff4c Compare November 25, 2025 19:44

mjuric force-pushed the tickets/DM-53310 branch 3 times, most recently from 78e8a4d to 7b11908 Compare November 25, 2025 20:00

andy-slac approved these changes Nov 25, 2025

View reviewed changes

mjuric force-pushed the tickets/DM-53310 branch from 7b11908 to 37e6f6d Compare December 1, 2025 03:37

isullivan approved these changes Dec 2, 2025

View reviewed changes

mjuric added 4 commits December 1, 2025 21:26

Update lsstcam.yaml SS* tables to RFC-1138 (DP2) schema

4ae2518

Update apdb.yaml SS* tables to RFC-1138 (DP2) schema

75b9a65

Also drop "@id" columns which can now be auto-generated.

Add news fragment for RFC-1138 (SS* table) changes

0606f69

Review comment implementation

3dc7f48

Gerenjie force-pushed the tickets/DM-53310 branch from 37e6f6d to 3dc7f48 Compare December 2, 2025 05:26

Gerenjie merged commit f775bcd into main Dec 2, 2025
15 checks passed

Gerenjie deleted the tickets/DM-53310 branch December 2, 2025 06:44

		description: This is a set of unique identifier_ids in an array that points to the identification_metadata
		table.

DM-53310: Implement RFC-1138 changes (solar system tables) #424

DM-53310: Implement RFC-1138 changes (solar system tables) #424

Uh oh!

Conversation

mjuric commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyMcCormick commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyMcCormick commented Nov 24, 2025

Uh oh!

mjuric commented Nov 24, 2025

Uh oh!

mjuric commented Nov 25, 2025

Uh oh!

JeremyMcCormick commented Nov 25, 2025

Uh oh!

JeremyMcCormick commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-slac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-slac commented Nov 25, 2025

Uh oh!

mjuric commented Nov 25, 2025

Uh oh!

mjuric commented Nov 25, 2025

Uh oh!

andy-slac commented Nov 25, 2025

Uh oh!

isullivan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjuric commented Nov 22, 2025 •

edited

Loading

JeremyMcCormick commented Nov 24, 2025 •

edited

Loading

JeremyMcCormick commented Nov 25, 2025 •

edited

Loading