You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "gsmiller (via GitHub)" <gi...@apache.org> on 2023/05/12 20:52:02 UTC
[GitHub] [lucene] gsmiller opened a new pull request, #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
gsmiller opened a new pull request, #12289:
URL: https://github.com/apache/lucene/pull/12289
### Description
Right now, `FilterTerms#intersect` is picking up the default implementation from `Terms`, which hides any custom implementation provided by the wrapped `Terms` instance. This is unfortunate since some `Terms` implementations provide optimized intersection logic (e.g., BlockTree's `FieldReader#intersect`). We should instead delegate to the wrapped `Terms` instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller closed pull request #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller closed pull request #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
URL: https://github.com/apache/lucene/pull/12289
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #12289:
URL: https://github.com/apache/lucene/pull/12289#issuecomment-1546288958
Note that `testOverrideMethods` is failing right now. I can update this test, but I wonder if there's a good reason we wouldn't want to make the change proposed here? Seems like we should, but open to feedback. Maybe I'm missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #12289:
URL: https://github.com/apache/lucene/pull/12289#issuecomment-1546614327
maybe simpler explanation, imagine a user if you delegated this: "I wrote a FilterTerms to remove curse words, and it works, except they can bypass it with a wildcard query!"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #12289:
URL: https://github.com/apache/lucene/pull/12289#issuecomment-1546611473
I think the idea behind the test failure is this: this is a very advanced method and if you want to create a FilterTerms, to you know, filter the terms with a custom enum, the default implementation will continue to be correct and continue to work.
but if FilterTerms delegates the `intersect` method, then in practice almost every subclass would need to `@Override` and re-implement it with different logic (probably looking like the default implementation!!!) to be correct.
Plus, if you are filtering Terms, its usually the case you are gonna customize the TermsEnum too. delegating this method would be confusing as it would just return the raw terms enum you are wrapping rather than your custom class.
In general it is only safe to just delegate this method directly if you aren't changing anything serious about the Terms instance, so that you can just "pass it thru" and get correct results. You can do this in your particular FilterTerms subclass if you know it is safe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #12289: FilterLeafReader$FilterTerms#intersect now delegates to wrapped Terms
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #12289:
URL: https://github.com/apache/lucene/pull/12289#issuecomment-1546690470
@rmuir thanks for the explanation. This makes total sense. The situation I was looking at is one where we're not interested in changing the terms at all, but just tracking some metrics around different API invocations. Looks like a case where we should really override `intersect` ourselves. Based on your description of more general use-cases, I agree that it doesn't make sense to change this behavior for users in general. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org