Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-41652: Part 2 of adding single-attribute foreign keys to DP0.2 #186

Merged
merged 5 commits into from Feb 6, 2024

Conversation

gpdf
Copy link
Collaborator

@gpdf gpdf commented Feb 5, 2024

Includes:

  • DM-42772: Add foreign-key links for MatchesTruth table
  • DM-42785: Improve annotation of times and durations
  • Better annotation of the attributes used in foreign-key relationships

@gpdf gpdf marked this pull request as draft February 5, 2024 18:21
@gpdf gpdf self-assigned this Feb 5, 2024
@gpdf gpdf marked this pull request as ready for review February 5, 2024 18:37
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick left a comment

Choose a reason for hiding this comment

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

I am wondering if this PR should be split up as it contains three sets of seemingly unrelated changes:

  • Adding an ER diagram for one of the schemas
  • Adding additional ivoa:ucd attributes
  • Adding additional FKs

The yml directory doesn't seem like an ideal place to put documentation, as there are only YAML files there now. A better organization might be putting these types of files in their own dedicated doc directory with its own structure like doc/schemas/$SCHEMA_NAME or something similar.

@gpdf
Copy link
Collaborator Author

gpdf commented Feb 5, 2024

I did separate the changes reasonably carefully into distinct self-contained commits. If you do want separate PRs we can do that, but it won't really change what shows up in the repo history.

I agree that the yml/ subdirectory is not the right place, but I thought of the .mmd as mostly just a communication to you to help you think about how to build a tool to auto-generate it, so it's not envisioned as a permanent place. How about if we leave it there for now and then talk to Jonathan this week about a strategy for properly documenting sdm_schemas? I don't know if that would naturally lead to it having its own standalone doc/ subdirectory or not.

@gpdf
Copy link
Collaborator Author

gpdf commented Feb 5, 2024

I'm also OK with just creating a doc/ now and figuring the rest out later.

@JeremyMcCormick
Copy link
Collaborator

I'm also OK with just creating a doc/ now and figuring the rest out later.

I do think it would be good to move this into a new doc directory. It's likely that whatever new documentation infrastructure we add in the future would use this as the base directory and we would not put these types of files under yml.

@gpdf gpdf force-pushed the tickets/DM-41652-part2 branch 2 times, most recently from ce3abb9 to aaeb11d Compare February 5, 2024 22:11
Add or update associated annotations to improve presentation
Add UCDs for IDs to support foreign-key links
Completes original scope of DM-41652
Add time.epoch or time.duration UCDs as appropriate.
Add units in one place where they were missing
Eventually we should try to generate this automatically from Felis.
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick left a comment

Choose a reason for hiding this comment

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

We discussed inclusion of three separate sets of changes in this PR and decided it was acceptable. I checked that the new IVOA UCD attributes seem to match conceptually the columns to which they are assigned. The new FKs were verified to load correctly into TAP_SCHEMA. There is a problem with the Felis sql module having to do with an ordering dependence on column definitions that occurs with these changes, but this is not a an issue with sdm_schemas itself, but rather something that needs to be fixed in Felis. Finally, the new example ERD diagram was moved into the doc directory. This may be used as a template for automatically generating these diagrams in the future.

@gpdf
Copy link
Collaborator Author

gpdf commented Feb 6, 2024

Thanks. Omnibus PRs are not my usual style anyway, so I'm happy to keep things separated in general.

@gpdf gpdf merged commit eb1d092 into main Feb 6, 2024
4 checks passed
@gpdf gpdf deleted the tickets/DM-41652-part2 branch February 6, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants