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

Should ExtendedPathIndex be usable for arbitrary paths? #12

Open
d-maurer opened this issue Mar 12, 2019 · 3 comments
Open

Should ExtendedPathIndex be usable for arbitrary paths? #12

d-maurer opened this issue Mar 12, 2019 · 3 comments

Comments

@d-maurer
Copy link

ExtendedPathIndex has some features which indicate that it targets arbitrary paths and not only those derived from getPhysicalPath:

  • the attribute name to be indexed can be specified
  • the index supports multi_valued.

If so, then the following optimisation is (likely) wrong:

        # Avoid using the root set
        # as it is common for all objects anyway and add overhead
        # There is an assumption about all indexed values having the
        # same common base path
        if level == 0:
            indexpath = [p for p in self.getPhysicalPath() if p]
            minlength = min(len(indexpath), len(comps))
            # Truncate path to first different element
            ...

As the comment indicates, the index assumes that all paths have a common base path. In the code above, it is heuristically determined based on getPhysicalPath. However, this is correct only when the paths are derived from getPhysicalPath and are not "arbitrary" paths.
The optimisation should therefore only be applied when the indexed attribute is getPhysicalPath

@d-maurer
Copy link
Author

As apparently "There is an assumption about all indexed values having the same common base path", why not use this systematically (rather than only during querying): make the common base path explicit and do not index that part (and verify that the assumption is met).

@jensens
Copy link
Sponsor Member

jensens commented Mar 16, 2020

I did not get what's the problem here. I don't think its a good idea to bind functionality to an attribute name.

It works well for an UUID based index in https://github.com/plone/plone.app.multilingualindexes/
Therefore it supports this since #8

@d-maurer
Copy link
Author

I did not get what's the problem here.

In the normal setup, ExtendedPathIndex indexes the objects under their full path, as given by getPhysicalPath. But, in this setup, those objects have a common path prefix: the path to the portal. The index tries to optimize this in some cases guessing a common prefix and removing it. This works only reliable when the path is indeed the one given by getPhysicalPath; it fails (and potentially gives wrong results) when the path is a path in a different hierarchy, e.g. a hierarchical thesaurus, a keyword hierarchy, etc..

I don't think its a good idea to bind functionality to an attribute name.
As it is now, the index does not prevent its use for hierarchies not associated with getPhysicalPath but uses (even in those cases) an optimization based on getPhysicalPath. I think this is not a good idea. Either the index should be tightly bound to getPhysicalPath or work reliably with other types of paths.

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

2 participants