BREAKING: Remove covariance and LINQ use from Grouping, #1059

[paulirwin]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Removes the covariance and LINQ use from Grouping, breaks GroupingSearch into subclasses

Fixes #1059

Description

See #1059 for rationale. This PR breaks GroupingSearch into three child classes (with some abstract base classes for common configuration properties) so that we can remove the covariance in the interfaces that were added to get this working in the original port. This keeps most of the interfaces but makes them non-generic (and thus, not covariant) for cases where you might need to have them in a common variable or collection. These were also needed for usage in the tests, where the test code randomly switches implementations that otherwise would not have common generic type parameters.

LINQ was removed, and IEnumerable<T> was changed to ICollection<T>/IList<T> to match the original Java code. This should improve performance a little bit by being able to reference Count etc. directly.

[paulirwin]

This old PR has been rebased against latest main. I intend to merge this this coming weekend if there is no further feedback.

[NightOwl888]

Not a full review, but there are some old comments that I had in a draft that I am posting so they aren't lost.

There was one issue I found on the first pass where it would have made sense to make a method into an extension method so it could expose the underlying type, but I was unable to locate it when I went back to find it again. I am hoping to locate it while this is still a PR.

This is going to take some dedicated time for a full review, though.

[Copilot]

Pull Request Overview

This PR removes covariance and LINQ usage from the Grouping module to align with the original Java code and improve performance. The main changes involve:

• Converting generic covariant interfaces to non-generic interfaces with explicit implementations
• Replacing IEnumerable<T> with ICollection<T>/IList<T> for better performance
• Adding casting adapter classes to bridge type conversions
• Breaking GroupingSearch into specialized subclasses

Reviewed Changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Support/CastingSetAdapter.cs	New adapter class for type-safe casting between set types
Support/CastingListAdapter.cs	New adapter class for type-safe casting between list types
Support/CastingEnumeratorAdapter.cs	New adapter class for type-safe casting between enumerator types
Support/CastingCollectionAdapter.cs	New adapter class for type-safe casting between collection types
Tests	Updated test files to use concrete types instead of covariant interfaces
Grouping	Refactored interfaces to be non-generic with explicit implementations

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[paulirwin]

After several iterations of review and improvement, I now think this PR is good to go. Below is the final PR review by Claude Code (Opus 4.6). My responses follow its review.

---

Code Review: Issue/1059 — Grouping API Refactoring (Final)

Summary

This PR refactors Lucene.Net.Grouping to eliminate Java generic type erasure / wildcard generic workarounds and replace them with idiomatic C#. The monolithic GroupingSearch is split into a static factory + three concrete subclasses, with a CRTP fluent API. A template method pattern (TwoPassGroupingSearch<T, TSelf>) extracts shared two-pass search orchestration. Old covariant generic interfaces are replaced with non-generic interfaces + casting adapters. LINQ/IEnumerable overhead is removed in hot paths.

---

Verdict: Approve

---

Strengths

• Clean factory pattern. GroupingSearch.ByField() / .ByFunction<T>() / .ByDocBlock() is discoverable and idiomatic C#.

• Template method pattern eliminates real duplication. TwoPassGroupingSearch<T, TSelf> encapsulates the entire two-pass orchestration (~115 lines), with four well-defined abstract factory methods. Each subclass only implements what differs.

• Class hierarchy is well-layered. GroupingSearch<T, TSelf> (settings) -> TwoPassGroupingSearch<T, TSelf> (template method) -> FieldGroupingSearch / FunctionGroupingSearch<T>. DocBlockGroupingSearch extends the base directly since it uses a single-pass block strategy.

• CRTP fluent API is correct. GroupingSearchFluentApiTest verifies every setter on every concrete type returns the correct type.

• Non-generic interfaces are a clean replacement. ITopGroups, IGroupDocs, ISearchGroup, IGroupingSearch, etc. provide type-erased access without the old covariant generic hacks.

• Casting adapters are well-designed. Minimal, correctly constrained (where T : U), placed in Lucene.Net.Support. CastingEnumeratorAdapter as readonly struct avoids heap allocations. CastingSetAdapter correctly uses .Cast<T>() for set operations.

• LINQ/IEnumerable cleanup is correct. IEnumerable<T> -> ICollection<T> and .Count() -> .Count removes unnecessary overhead.

• Documentation is updated. package.md examples use the new API. Migration guidance in AbstractDistinctValuesCollector is helpful.

• Test coverage is comprehensive. Fluent API tests, doc example validation tests, extended DistinctValuesCollectorTest, plus all existing tests updated.

---

Issues

Breaking Public API Without Compatibility Shim (Medium)

The old ITopGroups<T>, IGroupDocs<T>, ISearchGroup<T>, and IAbstractAllGroupsCollector<T> generic interfaces are removed entirely. Method signatures changed (e.g., GetTopGroups returns TopGroups<T> instead of ITopGroups<T>, constructors take ICollection<SearchGroup<T>> instead of IEnumerable<ISearchGroup<T>>). The BlockGroupingCollector.GetTopGroups non-generic overload is removed.

Clearly intentional — the migration remarks in AbstractDistinctValuesCollector confirm it. If this targets a major version bump, this is fine. If not, consider keeping old interfaces as [Obsolete] wrappers for one release cycle.

Unreachable Null Checks in Factory Methods (Low)

FieldGroupingSearch lines 142, 154, 166, 178 and FunctionGroupingSearch<T> lines 234, 246, 258, 270 all check if (groupField/groupFunction is null) throw IllegalStateException. These fields are readonly and set in the constructor. The null check can never be true at runtime — an ArgumentNullException in the constructor would catch the problem at the right time. This is carried over from the original Java pattern where the field was set conditionally. Not blocking, but worth cleaning up.

Narrow #nullable enable Scope (Nit)

In GroupingSearch.cs, #nullable enable is scoped to just DocBlockGroupingSearch (lines 279-310). This is correct for the object? usage there, but the rest of the file lacks nullable annotations. Consider a brief comment explaining why the scope is narrow, or enable it for the whole file.

---

Checklist

Security: No user input handling, injection vectors, or sensitive data changes.

Performance:

• .Count() -> .Count property
• IEnumerable<T> -> ICollection<T> avoids repeated enumeration
• CastingEnumeratorAdapter is readonly struct
• Casting adapters are lazy wrappers (no copying)

Testing:

• Happy path: field, function, doc block grouping
• Fluent API return types verified
• Documentation examples validated as runnable tests
• [SuppressCodecs("Lucene3x")] applied where SortedDocValuesField is used
• Randomized test framework properly handled

---

Overall

This is a high-quality refactoring. The class hierarchy is clean, the template method removes real duplication, test coverage is thorough, and documentation is updated. The breaking API changes are the main consideration — right design choice, but needs to be coordinated with versioning strategy.

---

My responses:

Breaking Public API Without Compatibility Shim (Medium)

This is intentional, and "only" breaks prior betas. While we don't relish in breaking changes, it is acceptable during the beta period, and is necessary to accomplish this refactoring. It is also called out as such in the PR title and label (which will put it in a Breaking Changes section of the release notes).

Unreachable Null Checks in Factory Methods (Low)

The parameter/field are not in a part of the file with nullable checking enabled, so it is possible that these are null in this case currently.

Narrow #nullable enable Scope (Nit)

I attempted to nullable-enable the entire file, but it would have resulted in an even larger scope for this PR, and there were several places where it was not immediately clear how to handle it. So to limit scope, I only applied it in this doc block case, so that we could ensure the resulting API was correctly annotated with nullability for its return type. (In this case, the group value is always null, so having it be typed to object? helps ensure that is handled appropriately by library users. FYI - this type parameter was the wildcard <?> in Java, and was also always null.)