Add Xapian Omega solution to haystack backend to fix long term issues#238
Add Xapian Omega solution to haystack backend to fix long term issues#238anarcat wants to merge 1 commit into
Conversation
Co-authored-by: Mark Sapiro <mark@msapiro.net>
|
Thanks, could you please also add tests for this functionality? |
|
well, i didn't write the original patch, and i have little familiarity with django development, let alone xapian or xapian-haystack... where would i begin? |
|
honestly, looking at the current test suite, it's going to be really hard for me to figure out what to do. i don't see anywhere where problematic corpus can be added, and even if there would be, i don't even know what our problematic corpus is. part of the problem is this bug doesn't seem to have been properly reported against xapian-haystack. or, more accurately, all related reports were closed (#153, #77)... I came from https://gitlab.com/mailman/hyperkitty/-/issues/408, which refered to those issues and the pull request in #181. I only submitted this PR because I was requested to rebase in #181, I didn't expect to be onboarded into writing unit tests here. :) That said, it looks like indexing a simple 255+ character string is enough to trigger the bug. In https://gitlab.com/mailman/hyperkitty/-/issues/408#note_1470192180, the following string is given as an example: I suspect this would be enough: 'x' * 255but i don't know where to plug this... |
|
Maybe @doctormo or any other follower could help with creating a test? |
|
i mentioned it on hyperkitty's side too, i'm kind of hoping @msapiro will jump in to save the day again ;) |
|
Actually, what's the situation here? The bugfix won't be accepted unless there are unit tests, even though it fixes real-world problems? That would be quite inconvenient... :) |
|
Just ran into this myself when indexing mailing lists with long lines/urls/... |
| NGRAM_MIN_LENGTH = getattr(settings, 'XAPIAN_NGRAM_MIN_LENGTH', 2) | ||
| NGRAM_MAX_LENGTH = getattr(settings, 'XAPIAN_NGRAM_MAX_LENGTH', 15) | ||
|
|
||
| LONG_TERM = re.compile(br'[^\s]{239,}') |
There was a problem hiding this comment.
Is this intentionally hard coded to 239, or should it depend on the LONG_TERM_LENGTH below?
|
I (like many people I assume) have a fork with this patch running (since 2021 in my case). I took a stab at writing a test. I hit a couple of snags that are worth documenting. I don't have time to reduce or open dedicated issues. Running tests locally is annoying because they're only really set up for Github actions but it's not impossible. Tests fail on macOS so I needed to fix that: https://tildegit.org/matthias/xapian-haystack/commit/4b0e6b3c74d5a7f7496eae775e9e610e4ae9c535 I wrote a basic regression test, but needed to decrease the XAPIAN_LONG_TERM_LENGTH since the test I chose to modify used This is obviously only a basic regression test (i.e. it crashes with "term too long" without the patch) and more thorough testing is possible (e.g. for retrieval or both truncating and non-truncating modes). I wish I had the time to get this over the finish line but I'm actually planning on switching off of Django for my current project so I'm not willing to invest more. But hopefully this is helpful for someone. My commits are public domain if anyone wants to cherry pick them. |
Reroll of #181 with @msapiro's tweak.