You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "chishui (via GitHub)" <gi...@apache.org> on 2025/07/24 06:24:36 UTC
[PR] Add accessors for MergeState's public fields [lucene]
chishui opened a new pull request, #14989:
URL: https://github.com/apache/lucene/pull/14989
### Description
MergeState class doesn't have good encapsulation and doesn't offer accessors for its fields. It makes mocking MergeState in unit tests difficult.
We can't modify the access level for these fields, so I add getter functions for all these public fields.
--
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
Re: [PR] Add accessors for MergeState and SegmentCommitInfo's public fields [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on code in PR #14989:
URL: https://github.com/apache/lucene/pull/14989#discussion_r2231747545
##########
lucene/core/src/java/org/apache/lucene/index/MergeState.java:
##########
@@ -91,7 +91,86 @@ public class MergeState {
/** Indicates if the index needs to be sorted * */
public boolean needsIndexSort;
- /** Sole constructor. */
+ /** Returns the document ID maps. */
+ public DocMap[] getDocMaps() {
+ return docMaps;
+ }
+
+ /** Returns the segment info of the newly merged segment. */
+ public SegmentInfo getSegmentInfo() {
+ return segmentInfo;
+ }
+
+ /** Returns the field infos of the newly merged segment. */
+ public FieldInfos getMergeFieldInfos() {
+ return mergeFieldInfos;
+ }
+
+ /** Returns the stored fields readers being merged. */
+ public StoredFieldsReader[] getStoredFieldsReaders() {
+ return storedFieldsReaders;
+ }
+
+ /** Returns the term vectors readers being merged. */
+ public TermVectorsReader[] getTermVectorsReaders() {
+ return termVectorsReaders;
+ }
+
+ /** Returns the norms producers being merged. */
+ public NormsProducer[] getNormsProducers() {
+ return normsProducers;
+ }
+
+ /** Returns the DocValues producers being merged. */
+ public DocValuesProducer[] getDocValuesProducers() {
+ return docValuesProducers;
+ }
+
+ /** Returns the field infos being merged. */
+ public FieldInfos[] getFieldInfos() {
+ return fieldInfos;
+ }
+
+ /** Returns the live docs for each reader. */
+ public Bits[] getLiveDocs() {
+ return liveDocs;
+ }
+
+ /** Returns the postings to merge. */
+ public FieldsProducer[] getFieldsProducers() {
+ return fieldsProducers;
+ }
+
+ /** Returns the point readers to merge. */
+ public PointsReader[] getPointsReaders() {
+ return pointsReaders;
+ }
+
+ /** Returns the vector readers to merge. */
+ public KnnVectorsReader[] getKnnVectorsReaders() {
+ return knnVectorsReaders;
+ }
+
+ /** Returns the max docs per reader. */
+ public int[] getMaxDocs() {
+ return maxDocs;
+ }
+
+ /** Returns the info stream for debugging messages. */
+ public InfoStream getInfoStream() {
+ return infoStream;
+ }
+
+ /** Returns the executor for intra merge activity. */
+ public Executor getIntraMergeTaskExecutor() {
+ return intraMergeTaskExecutor;
+ }
+
+ /** Returns whether the index needs to be sorted. */
+ public boolean getNeedsIndexSort() {
+ return needsIndexSort;
+ }
Review Comment:
Adding a getter to a public field is senseless java verbosity: it adds no value. The field is public: just access the field directly.
--
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
Re: [PR] Add accessors for MergeState and SegmentCommitInfo's public fields [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14989:
URL: https://github.com/apache/lucene/pull/14989#issuecomment-3408720142
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Add accessors for MergeState and SegmentCommitInfo's public fields [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14989:
URL: https://github.com/apache/lucene/pull/14989#issuecomment-3181730120
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Add accessors for MergeState and SegmentCommitInfo's public fields [lucene]
Posted by "chishui (via GitHub)" <gi...@apache.org>.
chishui commented on code in PR #14989:
URL: https://github.com/apache/lucene/pull/14989#discussion_r2238326161
##########
lucene/core/src/java/org/apache/lucene/index/MergeState.java:
##########
@@ -91,7 +91,86 @@ public class MergeState {
/** Indicates if the index needs to be sorted * */
public boolean needsIndexSort;
- /** Sole constructor. */
+ /** Returns the document ID maps. */
+ public DocMap[] getDocMaps() {
+ return docMaps;
+ }
+
+ /** Returns the segment info of the newly merged segment. */
+ public SegmentInfo getSegmentInfo() {
+ return segmentInfo;
+ }
+
+ /** Returns the field infos of the newly merged segment. */
+ public FieldInfos getMergeFieldInfos() {
+ return mergeFieldInfos;
+ }
+
+ /** Returns the stored fields readers being merged. */
+ public StoredFieldsReader[] getStoredFieldsReaders() {
+ return storedFieldsReaders;
+ }
+
+ /** Returns the term vectors readers being merged. */
+ public TermVectorsReader[] getTermVectorsReaders() {
+ return termVectorsReaders;
+ }
+
+ /** Returns the norms producers being merged. */
+ public NormsProducer[] getNormsProducers() {
+ return normsProducers;
+ }
+
+ /** Returns the DocValues producers being merged. */
+ public DocValuesProducer[] getDocValuesProducers() {
+ return docValuesProducers;
+ }
+
+ /** Returns the field infos being merged. */
+ public FieldInfos[] getFieldInfos() {
+ return fieldInfos;
+ }
+
+ /** Returns the live docs for each reader. */
+ public Bits[] getLiveDocs() {
+ return liveDocs;
+ }
+
+ /** Returns the postings to merge. */
+ public FieldsProducer[] getFieldsProducers() {
+ return fieldsProducers;
+ }
+
+ /** Returns the point readers to merge. */
+ public PointsReader[] getPointsReaders() {
+ return pointsReaders;
+ }
+
+ /** Returns the vector readers to merge. */
+ public KnnVectorsReader[] getKnnVectorsReaders() {
+ return knnVectorsReaders;
+ }
+
+ /** Returns the max docs per reader. */
+ public int[] getMaxDocs() {
+ return maxDocs;
+ }
+
+ /** Returns the info stream for debugging messages. */
+ public InfoStream getInfoStream() {
+ return infoStream;
+ }
+
+ /** Returns the executor for intra merge activity. */
+ public Executor getIntraMergeTaskExecutor() {
+ return intraMergeTaskExecutor;
+ }
+
+ /** Returns whether the index needs to be sorted. */
+ public boolean getNeedsIndexSort() {
+ return needsIndexSort;
+ }
Review Comment:
Thanks for reviewing. I fully understand that you can just use public fields but that's not the point. Like what I mentioned in the description, it provides better testability, we can easily mock a member function but we can't mock a field access.
Apart from that, it's not good practice to have public fields as it doesn't provide good encapsulation. It's just the fields are already public and for backward compatibility, we can't change them to private.
--
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
Re: [PR] Add accessors for MergeState's public fields [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14989:
URL: https://github.com/apache/lucene/pull/14989#issuecomment-3116914107
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.
--
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
Re: [PR] Add accessors for MergeState's public fields [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14989:
URL: https://github.com/apache/lucene/pull/14989#issuecomment-3112203621
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.
--
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
Re: [PR] Add accessors for MergeState and SegmentCommitInfo's public fields [lucene]
Posted by "chishui (via GitHub)" <gi...@apache.org>.
chishui closed pull request #14989: Add accessors for MergeState and SegmentCommitInfo's public fields
URL: https://github.com/apache/lucene/pull/14989
--
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