search

sdss / valis

the SDSS API
BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

Include parent catalog associations in /target/catalogs endpoint #37

Closed albireox closed 3 months ago

albireox commented 3 months ago

Closes #31 ~and potentially #33~.

havok2063 commented 3 months ago

This looks good to me now. I'll approve. Feel free to merge now or after if you want look into the attr thing.

albireox commented 3 months ago

So no, the attr parameter doesn't seem to work. It only seems to work if the model that you're querying has a foreign key already defined to the joined table; in that case the attr seems to rename the FK attribute. But since we are joining with on= the parent catalog columns are not added to the model. When you do a .dicts() they are included, but flattened at the top level.

Instead I'm doing a single query with an additional join to sdss_id_to_catalogs which returns all the parent catalogue keys, and then I'm rearranging it for the response taking advantage that the parent catalogue columns can be separated using the double underscore. It's ... dict-comprehension heavy. Let me know what you think.

Interestingly the starfields(s) only works if it's the last element in the select. Otherwise it gets ignored even if the raw query is valid ... seems like a bug in how Peewee parses the results.

havok2063 commented 3 months ago

I'm ok with the dict comprehensions, as long as it doesn't slow the response time down of the route. If queries are all relatively small, then it sounds like it won't make much of a difference.

Interestingly the starfields(s) only works if it's the last element in the select. Otherwise it gets ignored even if the raw query is valid ... seems like a bug in how Peewee parses the results.

That is interesting, and also not surprising with peewee.

albireox commented 3 months ago

I think the difference in performance is none since if the attr argument worked Peewee would still need to do something similar internally, and this is just 10-12 keys/catalogues and a 2-3 catalogids per sdss_id so this should all be in the few milliseconds for doing the rearranging of the returned information.

I'll go ahead and merge this.