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

catalog query with sort_on="start" gives wrong order #1

Open
vincentfretin opened this issue Mar 13, 2012 · 9 comments
Open

catalog query with sort_on="start" gives wrong order #1

vincentfretin opened this issue Mar 13, 2012 · 9 comments

Comments

@vincentfretin
Copy link
Member

I know two places where you have sort_on="start" in a catalog query in Plone, in events and calendar portlets.
I have a events portlet in a production site with start index of type DateRecurringIndex.
I randomly see the events in the wrong order.

My guess is that the result.sort() line at line 738 in Products/ZCatalog/Catalog.py
doesn't work with the values from DateRecurringIndex._unindex which are IISet.

With DateIndex, result contains:

[(1078236420, -208989251, <bound method Catalog.__getitem__ of <Products.ZCatalog.Catalog.Catalog object at 0x85f2b90>>), (1077970020, -208989249, <bound method Catalog.__getitem__ of <Products.ZCatalog.Catalog.Catalog object at 0x85f2b90>>)]

with DateRecurringIndex, result contains:

[(IISet([1077998820]), 1208154628, <bound method Catalog.__getitem__ of <Products.ZCatalog.Catalog.Catalog object at 0x81d5488>>), (IISet([1078236420]), 1208154629, <bound method Catalog.__getitem__ of <Products.ZCatalog.Catalog.Catalog object at 0x81d5488>>)]

I think the comparison of two IISet is undefined. I didn't find any cmp in the code.

The solution is maybe to implement the documentToKeyMap method and return a fake object implementing getitem which return the first element of the sorted IISet...
ah no, that's not right if you have recurrence. I really don't know.

For now, I simply monkey patch the events portlet renderer to sort the date a posteriori.

results = catalog(portal_type='Event',                                      
               review_state=state,                                          
               end={'query': DateTime(),                                    
                    'range': 'min'},                                        
               path=path,                                                   
               )                                                            
#                   sort_on='start',                                            
#                   sort_limit=limit)[:limit]                                   
return sorted(results, key=lambda x: x.start)[:limit]
@thet
Copy link
Member

thet commented Mar 14, 2012

Hm.... the IISet as return value seem like a bug to me. Since the DateRecurringIndex should be a drop-in replacement of DateIndex, the return values shouldn't defer. I have to dig into this. But right away, I do not have an answer to this.

@thet
Copy link
Member

thet commented Mar 14, 2012

Regarding IISets: We probably should use IITreeSet instead to support get support for queryplan and the like. See Hannos answer in the recurring event index discussion:

"""
For example using IITreeSets instead of IISets, support for queryplan
and taking advantage of limited result sets and making sure to compare
prior indexed data with new data to avoid persistent changes.
"""
http://plone.293351.n2.nabble.com/Recurring-Event-Index-was-plone-app-event-spontaneous-sprint-tp6469177p6473374.html

@jensens
Copy link
Member

jensens commented Apr 2, 2012

+1 for using IITreeSets. At the time I used the IISets here, there was no queryplan written and most other Indexes used IISets too.

seanupton added a commit that referenced this issue Sep 10, 2012
@seanupton
Copy link
Member

It appears expedient to store a tuple for reverse/unindex entry values instead of an IISet, it seems to solve the sorting problem (FWIW, KeywordIndex uses a list). In the one case it appears needed, the tuple value is cast/iterated into an IISet local at runtime. No new tests, but no regressions either.

@seanupton
Copy link
Member

I am going to close this, if anyone sees need to re-open, do so and ping me.

@seanupton
Copy link
Member

Added note to changelog (docs/HISTORY.rst) explaining that any user who wants the added sorting functionality working should reindex the index see commit 0428f11

@seanupton seanupton reopened this Sep 11, 2012
@seanupton
Copy link
Member

Okay, so while this is solved, reindexing is not sufficient for migrating existing sites... there either needs to be a removal and re-add of each index using this index type, or something programmatic -- I did this in a debug session, but maybe this needs a more formal solution -- or at least better documentation of this migration quirk:

>>> from zope.component.hooks import setSite
>>> import transaction
>>> site = app.Plone  # use your sitename here
>>> setSite(p)
>>> app._p_jar.sync()
>>> rev_start = site.portal_catalog._catalog.indexes['start']._unindex
>>> rev_start = site.portal_catalog._catalog.indexes['start']._unindex
>>> rev_end = site.portal_catalog._catalog.indexes['end']._unindex
>>> for k,v in rev_end.items():
...     rev_end[k] = tuple(v)
... 
>>> for k,v in rev_start.items():
...     rev_start[k] = tuple(v)
... 
>>> txn = transaction.get()
>>> txn.note('/'.join(site.portal_catalog.getPhysicalPath()))
>>> txn.note('Reindexed start and end indexes (replaced reverse index values programmatically)')
>>> txn.commit()

@thomasmassmann
Copy link
Member

How should it even be possible to sort this, when Catalog and Index have no information about query params. Example: get all events with an end date in the future, sort by start date. It will put recurring events on top of the list when the first occurrence of the event was before the start date of a regular event, no matter what.

@thomasmassmann
Copy link
Member

@jensens, @thet: I think it is currently not possible to sort by start/end date, or am I missing something here? Do we have an alternative solution?

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

No branches or pull requests

5 participants