You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/07/05 07:08:13 UTC
[GitHub] [lucene] zacharymorn opened a new pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
zacharymorn opened a new pull request #205:
URL: https://github.com/apache/lucene/pull/205
# Description
Rename `TermVectorsReader` to `TermVectorsReaderBase` in preparation for introducing `TermVectors` and new `TermVectorsReader` abstractions. Please see https://github.com/apache/lucene/pull/180#issuecomment-871155896 for details.
Please note that relevant variable names were currently not renamed, in order to keep the changes minimal and reduce the need to undo the renaming once `TermVectorsReader` is introduced back.
# Tests
Passed existing tests.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/lucene/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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] zacharymorn commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r665060449
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
Yeah this indeed looks inconsistent. I think the main thing here is, the overall understanding I got from https://github.com/apache/lucene/pull/180#discussion_r650720327 & https://github.com/apache/lucene/pull/180#discussion_r653357562 is that the new `TermVectors / TermVectorsReader` abstraction is supposed to be an index API, while the original `TermVectorsReader` / now `TermVectorsReaderBase` is a codec API, which has `clone/getMergeInstance/checkIntegrity` methods, and also implements `Closeable` (and all other `*Reader/Producer` codec classes also implement `Closeable` as well). If we were to move `Closeable` from `TermVectorsReaderBase` to the new `TermVectorsReader`, wouldn't that break the separation there?
--
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] dsmiley commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r666242176
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
I just left a [comment](https://github.com/apache/lucene/pull/180#issuecomment-876482149) back on #180 to suggest merging that PR as-is and why.
--
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] zacharymorn commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r664278681
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
> In the subsequent PR that re-introduces "TermVectorsReader", I assume you will change some of the references of TermVectorsReaderBase back to TermVectorsReader
Yes I'm planning to do that!
> such as right here?
For this particular one though, later in the code its `close` method is called via:
https://github.com/apache/lucene/blob/167bd99c23520f8e252ad6f98d1e3065d20d5ae6/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java#L185-L192
So it may still need to use `TermVectorsReaderBase` there, as that class still implements `Closeable` per https://github.com/apache/lucene/pull/180#issuecomment-871155896 ?
--
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] dsmiley commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r664625324
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
Perhaps then TermVectorsReader should also implement Closeable? (to be done in another PR)
I called out the line above because it is not harmonious with the other "*Reader" suffixed classes declared next to it.
--
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] dsmiley commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r665342391
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
Sigh; I'm not sure what to do then. The non-harmoneous aspect is a code smell but if it's very limited (not widespread) then it's not a big deal I guess. Otherwise it's a stronger indicator there is misalignment. I'm not really sure why there needs to be _both_ TermVectorsReader & TermVectorsReaderBase any way but surely you & Adrien do so I leave that to your judgement.
--
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] zacharymorn closed pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn closed pull request #205:
URL: https://github.com/apache/lucene/pull/205
--
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] dsmiley commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r664111224
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
In the subsequent PR that re-introduces "TermVectorsReader", I assume you will change some of the references of TermVectorsReaderBase back to TermVectorsReader, such as right here?
--
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] dsmiley commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r664111224
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
In the subsequent PR that re-introduces "TermVectorsReader", I assume you will change some of the references of TermVectorsReaderBase back to TermVectorsReader, such as right here?
--
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] zacharymorn commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r665966533
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
> I'm not really sure why there needs to be both TermVectorsReader & TermVectorsReaderBase any way but surely you & Adrien do so I leave that to your judgement.
The main reason that we are adding a new index level API for TermVectors (`TermVectors` in the other PR) is that we would like to provide a new way of TV access from `IndexReader` like the following
```
TermVectors termVectors = reader.getTermVectorsReader(); // would clone the underlying TermVectorsReader to be
thread-safe, and GC-able once usage is finished
for (int docId = 0; docId < numDocs; docId++) {
Fields vectors = termVectors.get(docId);
}
```
to replace the old way of access in the form of
```
for (int docId = 0; docId < numDocs; docId++) {
Fields vectors = reader.getTermVectors(docId); // using thread-local to cache
}
```
as per https://github.com/apache/lucene/pull/137#issuecomment-840111367 . From my understanding, it's a bit like separating out the existing index level API `IndexReader#getTermVectors` into a new dedicated class to handle TV (and we do plan to deprecate the existing API once the new one is ready), and hence some of the internal index / codec classes organization details are now exposed via the indirection and inheritance relationships, and we need a bridge between them (by having codec class `TermVectrosReaderBase` to inherit the new index class `TermVectorsReader`).
Not sure if @jpountz or @rmuir have any further suggestion on this? If there's no strong objection to the current approach in this PR and the other one https://github.com/apache/lucene/pull/180, we can potentially use https://issues.apache.org/jira/browse/LUCENE-10018 to further enhance on this?
--
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] zacharymorn commented on pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on pull request #205:
URL: https://github.com/apache/lucene/pull/205#issuecomment-874499672
> I'm also okay with introducing the new/simpler TermVectorsReader abstraction if it would reduce this PR diff a lot. Up to you.
Thanks @dsmiley for the review and approval! I've thought about having the new `TermVectorsReader` here as well, but I'm leaning more towards introducing it in the other PR after some digging, as I see so far the majority of `TermVectorsReaderBase` usage here involves using the existing codec APIs `checkIntegrity/clone/getMergeInstance`, so the newer `TermVectorsReader` class (that may not have any method yet) may not be usable directly. In addition, introducing it here may also lead to some git merge conflicts with the other PR later as well I feel, as `TermVectors` was already created there as the super class of `TermVectorsReader`. So I think saving it for the other PR may be an easier approach overall?
--
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] dsmiley commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r665343965
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
Maybe another idea is leave TermVectorsReader be. In the other PR you have, rename TermVectors to TermVectorsSupplier? Just a thought for your consideration; I have no strong opinion.
--
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] jpountz commented on pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
jpountz commented on pull request #205:
URL: https://github.com/apache/lucene/pull/205#issuecomment-883355855
I prefer the current API that's been merged as part of #180 than the proposed API where Fields, TermVectors and TermVectorsReader get renamed to TermVectors, TermVectorsReader and TermVectorsReaderBase. Removing usage of `Fields` doesn't help if we're introducing a new `TermVectorsReaderBase` abstraction instead?
--
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] zacharymorn commented on pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on pull request #205:
URL: https://github.com/apache/lucene/pull/205#issuecomment-885985951
Sorry I have been away for a few days. I should have closed this PR after the merge of https://github.com/apache/lucene/pull/180, as we (@dsmiley and I) also agreed earlier that the renaming may not be a good approach for the problem we are trying to solve here, sorry for the delay there as well.
--
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 #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #205:
URL: https://github.com/apache/lucene/pull/205#issuecomment-882345193
-1 to these renames that solve no api problems.
--
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] zacharymorn commented on pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on pull request #205:
URL: https://github.com/apache/lucene/pull/205#issuecomment-874499672
> I'm also okay with introducing the new/simpler TermVectorsReader abstraction if it would reduce this PR diff a lot. Up to you.
Thanks @dsmiley for the review and approval! I've thought about having the new `TermVectorsReader` here as well, but I'm leaning more towards introducing it in the other PR after some digging, as I see so far the majority of `TermVectorsReaderBase` usage here involves using the existing codec APIs `checkIntegrity/clone/getMergeInstance`, so the newer `TermVectorsReader` class (that may not have any method yet) may not be usable directly. In addition, introducing it here may also lead to some git merge conflicts with the other PR later as well I feel, as `TermVectors` was already created there as the super class of `TermVectorsReader`. So I think saving it for the other PR may be an easier approach overall?
--
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] zacharymorn commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r665060449
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
Yeah this indeed looks inconsistent.
I think the main thing here is, the overall understanding I got from https://github.com/apache/lucene/pull/180#discussion_r650720327 & https://github.com/apache/lucene/pull/180#discussion_r653357562 (this comment specifically asked not to move up `Closeable` from the original `TermVectorsReader`) is that, the new `TermVectors / TermVectorsReader` abstraction is supposed to be an index API, while the original `TermVectorsReader` / now `TermVectorsReaderBase` is a codec API, which has `clone/getMergeInstance/checkIntegrity` methods, and also implements `Closeable` (and all other `*Reader/Producer` codec classes also implement `Closeable` as well). If we were to move `Closeable` from `TermVectorsReaderBase` to the new `TermVectorsReader`, wouldn't that break the separation there?
--
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] zacharymorn commented on a change in pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on a change in pull request #205:
URL: https://github.com/apache/lucene/pull/205#discussion_r664278681
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
##########
@@ -57,7 +57,7 @@
final NormsProducer normsProducer;
final StoredFieldsReader fieldsReaderOrig;
- final TermVectorsReader termVectorsReaderOrig;
+ final TermVectorsReaderBase termVectorsReaderOrig;
Review comment:
> In the subsequent PR that re-introduces "TermVectorsReader", I assume you will change some of the references of TermVectorsReaderBase back to TermVectorsReader
Yes I'm planning to do that!
> such as right here?
For this particular one though, later in the code its `close` method is called via:
https://github.com/apache/lucene/blob/167bd99c23520f8e252ad6f98d1e3065d20d5ae6/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java#L185-L192
So it may still need to use `TermVectorsReaderBase` there, as that class still implements `Closeable` per https://github.com/apache/lucene/pull/180#issuecomment-871155896 ?
--
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] dsmiley commented on pull request #205: LUCENE-9959: Rename TermVectorsReader to TermVectorsReaderBase
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #205:
URL: https://github.com/apache/lucene/pull/205#issuecomment-883540136
FWIW, what I care about most in this is that Fields.java is relegated to an internal only thing associated with the terms index, for those that build PostingsFormats 'n such. I find it confusing to have this Fields class used in different ways across the codebase -- the PostingsFormat related Fields, and the term vector Fields. I care less about what we name some of the things, or wether a new public class is needed somewhere.
--
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