NightOwl888 commented on code in PR #818:
URL: https://github.com/apache/lucenenet/pull/818#discussion_r1166129717
##########
src/Lucene.Net/Search/IndexSearcher.cs:
##########
@@ -160,8 +160,18 @@ public IndexSearcher(IndexReaderContext context)
/// Expert: Creates an array of leaf slices each holding a subset of
the given leaves.
/// Each <see cref="LeafSlice"/> is executed in a single thread. By
default there
/// will be one <see cref="LeafSlice"/> per leaf (<see
cref="AtomicReaderContext"/>).
+ ///
+ /// NOTE: When overriding this method, be aware that the constructor
of this class calls
+ /// a private method and not this virtual method. So if you need to
override
+ /// the behavior during the initialization, call your own private
method from the constructor
+ /// with whatever custom behavior you need.
/// </summary>
- protected virtual LeafSlice[] Slices(IList<AtomicReaderContext> leaves)
+ // LUCENENET specific - renamed to GetSlices to better indicate the
purpose of this method
+ protected virtual LeafSlice[] GetSlices(IList<AtomicReaderContext>
leaves)
+ => GetSlicesInternal(leaves);
+
+ // LUCENENET specific - creating this so that we can call it from the
constructor
+ protected LeafSlice[] GetSlicesInternal(IList<AtomicReaderContext>
leaves)
Review Comment:
Oops, looks like this wasn't made `private`.
But after taking a fresh look at this, I see that the `m_leafSlices` field
the constructor sets is already protected. A subclass would simply need to pass
`null` for `executor` and they would be able to call
`m_leafSlices.GetSlices(context. Leaves)` themselves.
The only limitation is that they wouldn't be able to set `executor` because
it is marked private, so it wouldn't go down the execution path that actually
uses `m_leafSlices`.
The current solution calls `GetSlicesInternal()` and sets `m_leafSlices`,
which is an array allocation. The subclass has no control over this. Then, if
they wanted to set state before it is called, the only choice is to allocate
this array again by calling `GetSlices()` from the subclass constructor. So,
not only would it need to be called twice, the overridden `GetSlices()` method
would need to account for this.
I think a better solution would be to add a protected constructor with the
ability to opt out of the `GetSlices()` call. Then they can set any state
passed in from their constructor prior to calling `GetSlices()` and there is
only one allocation.
```c#
protected IndexSearcher(IndexReaderContext context, TaskScheduler executor,
bool allocateLeafSlices)
```
Of course, we need to make it very clear that when passing `false` for
`allocateLeafSlices` they will need to do it in their constructor. It would
also be good to put guard clauses on all usages of `m_leafSlices` so that if it
is `null` during search the user should get an `InvalidOperationException`
explaining that `m_leafSlices` must be set in the constructor (and it must be
there since it is readonly). Of course, it would also be helpful to mention
they can set it by calling the following in the constructor documentation.
```c#
this.m_leafSlices = GetSlices(m_leafContexts);
```
Given that this is one of the main APIs of this library, I think it is worth
the diligence to get it right. If you can come up with a more elegant solution
than this, feel free to suggest 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: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
