Kian-Tat Lim added a comment - 07/Jan/22 4:57 PM Ignoring the order_by and limit parameters in https://github.com/lsst/daf_butler/blob/main/python/lsst/daf/butler/registry/queries/_results.py#L159 looks suspicious but maybe not the proximate cause of this.

Andy Salnikov added a comment - 10/Jan/22 5:38 PM Eli Rykoff , thanks for the script. Indeed, current implementation does not scale too well, it is related to how we build sub-queries and propagate ordering. I'll try to find a better way to do that.

Andy Salnikov added a comment - 11/Jan/22 5:09 AM I decided that dragging ORDER BY columns from a sub-query to a parent query (or materialized query) is too much effort, so instead I add a computed column to the result of a query which contains rank of a record. That rank is calculated using SQL window function as row_number() OVER (ORDER BY columns) . It is much easier to handle a single column with a fixed name. The query that we generate for queryDimensionRecords will now look like: SELECT main_20210215.exposure.instrument, main_20210215.exposure.id, -- ... more exposure columns main_20210215.exposure.zenith_angle, main_20210215.exposure.timespan, c._orderby AS _orderby FROM main_20210215.exposure JOIN ( SELECT main_20210215.physical_filter.band AS band, main_20210215.physical_filter.instrument AS instrument, main_20210215.physical_filter. name AS physical_filter, main_20210215.exposure.id AS exposure, row_number() OVER ( ORDER BY main_20210215.exposure.day_obs ASC ) AS _orderby -- this is the new column FROM ( SELECT main_20210215.dataset_tags_00000002.instrument AS instrument, main_20210215.dataset_tags_00000002.detector AS detector, main_20210215.dataset_tags_00000002.exposure AS exposure FROM main_20210215.dataset_tags_00000002 WHERE main_20210215.dataset_tags_00000002.dataset_type_id = %(dataset_type_id_1)s AND main_20210215.dataset_tags_00000002.collection_name = %(collection_name_1)s ) AS raw JOIN main_20210215.exposure ON raw.instrument = main_20210215.exposure.instrument AND raw.exposure = main_20210215.exposure.id JOIN main_20210215.physical_filter ON raw.instrument = main_20210215.physical_filter.instrument AND main_20210215.exposure.instrument = main_20210215.physical_filter.instrument AND main_20210215.exposure.physical_filter = main_20210215.physical_filter. name WHERE main_20210215.exposure.day_obs > %(param_1)s AND raw.instrument = %(instrument_1)s AND main_20210215.exposure.instrument = %(instrument_2)s AND main_20210215.physical_filter.instrument = %(instrument_3)s ORDER BY _orderby ) AS c ON main_20210215.exposure.instrument = c.instrument AND main_20210215.exposure.id = c.exposure ORDER BY _orderby New column _orderby is used for ordering in both sub-query and outer query. It is not strictly needed in a sub-query if LIMIT is not used, but I still keep it there (my idea that in many cases when ORDER BY is used LIMIT will also be present). This only works correctly if the is only one sub-query (no UNIONs or joins with other JOINs) which I believe is true for queryDimensionRecords().

Andy Salnikov added a comment - 11/Jan/22 5:32 PM Looks like I was too optimistic about my solution. It does work for some cases, but with many possible combinations of DataCoordinateIterable and DimensionRecordStorage implementations it is not possible to make DimensionRecordStorage.fetch() return ordered result set. fetch() is actually very explicit about that it should not be expected to return ordered result, but I was hoping that I could reverse that. Unfortunately I realize now that it would open another can of worms which looks nasty. I think I have to go back to my previous solution of sorting results on client side but try to solve that scalability issue using different approach.

Andy Salnikov added a comment - 12/Jan/22 3:11 AM - edited Jim Bosch , I think this is ready for review. I decided to keep window function in DataIds query (simplifies things a bit) but for DatasetRecords I'm doing in-memory ordering. Jenkins is OK, Github Actions was fine for my previous commit but failed for the last one (and last change was only in commit message). I saw people complaining about GA failures, will try to re-run it later today or tomorrow. (EDIT: GA is OK now after re-run)

Jim Bosch added a comment - 13/Jan/22 5:32 PM Looks good; I had basically nothing to say on the PR except for a summary asking for a follow-up ticket. I believe it is only possible for UNIONs and subqueries with joins of their own to appear in queryDatasets (which doesn't support ORDER BY), so I agree that the limitations involving those are okay.

Andy Salnikov added a comment - 13/Jan/22 6:54 PM Thanks for review, merged. Merlin Fisher-Levine , with a just merged main your script should work much faster now, second(s) instead of minutes.

Eli Rykoff added a comment - 13/Jan/22 11:10 PM This is great! I just checked the test code that I had and it takes about 1.1 s on lsst-devl03 with the new daf_butler.