NightOwl888 commented on issue #1059:
URL: https://github.com/apache/lucenenet/issues/1059#issuecomment-2543953460
> > It looks like you neatly solved access to the GroupCount field using
IGroupCountCollector, but the tests don't access the Groups property so there
is no need to find a solution to cast it AFAICT.
>
> I actually already removed that in lieu of having this do the same
approach as the other collectors in the tests, so that we're not adding an
unnecessary interface just for testing purposes. It's unlikely that someone
would need just the group counts in the real world without also accessing the
groups, so I figured it would be best to not pollute the public API with that
interface when it was only used (read: not needed, just used as one approach)
for test purposes.
-----------------
> I just had an idea, based on that last comment, if you're open to it. We
could create non-generic interfaces (not abstract classes) that do not expose
collections, but give you APIs to retrieve items. By using an interface instead
of an abstract class, we wouldn't be forced to convert to/from object except
for in the explicitly implemented methods. It also would not change the type
hierarchy. For example:
>
> ```cs
> public interface IAllGroupsCollector : ICollector // NOTE: we'd need to
keep this interface
> {
> int GroupCount { get; }
> object GetGroup(int index);
> }
> ```
>
> We would then explicitly implement the interface for the GetGroup method.
I think that would be cleaner than exposing a non-generic ICollection, although
I could be swayed on explicitly implementing that too. Thoughts?
That is the reason why I said "abstraction" rather than "abstract class",
because either would probably be fine. I just used abstract classes in most
other places because it meant keeping the same syntax as the upstream code. For
example:
#### Java
```java
List<AbstractAllGroupsCollector> collectors = new ArrayList();
ValueSource vs = new BytesRefFieldSource(groupField);
collectors.add(new FunctionAllGroupHeadsCollector(vs, new Map(),
sortWithinGroup));
collectors.add(TermAllGroupHeadsCollector.Create(groupField,
sortWithinGroup));
```
#### C#
```c#
List<AbstractAllGroupsCollector> collectors = new
List<AbstractAllGroupsCollector>();
ValueSource vs = new BytesRefFieldSource(groupField);
collectors.Add(new FunctionAllGroupHeadsCollector(vs, new Hashtable(),
sortWithinGroup));
collectors.Add(TermAllGroupHeadsCollector.Create(groupField,
sortWithinGroup));
```
Where if we used an interface, it would be declared a bit different:
```c#
List<IAllGroupsCollector> collectors = new List<IAllGroupsCollector>();
```
The main reason for doing this in other places was for being able to access
constants and fields without having to specify the non-generic type (in which
case the syntax stays the same as upstream).
It would also allow the use of the `Collector` abstract class, so the user
could cast to that type just as they could in Java. Being that there are no
methods on `ICollector` that wouldn't also exist on `Collector`, that is not
such a big deal, though.
That being said, using an interface does have an advantage we may be able to
take advantage of: it can be declared internal and used just for testing
purposes. If we use an abstract class, it will have to be declared public in
order for a public type to inherit it. Although, I am not entirely convinced
that getting the `GroupCount` without having strongly-typed access to the
groups is not a valid use case.
After mulling this over a bit more, if the user needed to create a
collection of mixed collectors, they could just declare it `object` or
`ICollector` and then pass it through a set of cast tests upon usage. However,
they would need to know all possible types that it could be cast to at compile
time, which might not be possible for every use case.
```c#
//List<AbstractGroupingSearch> collectors = new
List<AbstractGroupingSearch>();
List<object> collectors = new List<object>();
collectors.Add(GroupingSearch.ByField(...));
collectors.Add(GroupingSearch.ByFunction<MutableValueStr>(...));
collectors.Add(GroupingSearch.ByFunction<MutableValueInt32>(...));
collectors.Add(GroupingSearch.ByDocBlock<string>(...));
collectors.Add(GroupingSearch.ByDocBlock<int>(...));
foreach (object collector in collectors)
{
if (collector is FieldGroupingSearch fieldGroupingSearch)
// do something with the collector
else if (collector is FunctionGroupingSearch<MutableValueStr>
functionOnString)
// do something with the collector
else if (collector is FunctionGroupingSearch<MutableValueInt32>
functionOnInt32)
// do something with the collector
else if (collector is DocBlockGroupingSearch<string> docBlockOnString)
// do something with the collector
else if (collector is DocBlockGroupingSearch<int> docBlockOnInt32)
// do something with the collector
}
```
This would be very cumbersome if all they want to do is read a field that is
common among all of these types and even moreso if they wanted to do that for
any custom subclass (which would probably require reflection).
I am not opposed to keeping `ICollector`. The only thing that is kind of
smelly about it is that there is one constant that is declared on `Collector`
because interfaces in c# don't support constants. So, we need the `Collector`
class whether it is made abstract or not. Since it is currently a static class,
users could be confused that they cannot cast a collector type to `Collector`
as is the case in Java.
--
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
