Implement IEnumerable/IEnumerator in AbstractAppendingInt64Buffer, #279

[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.

Implement IEnumerable/IEnumerator in AbstractAppendingInt64Buffer

Fixes #279 (as the final remaining item)

Description

This implements IEnumerable properly in AbstractAppendingInt64Buffer, and updates the usages of the prior GetIterator method to use the enumerator.

[Copilot]

Pull request overview

Implements .NET enumeration support for AbstractAppendingInt64Buffer (so it can be used with foreach/LINQ) and updates internal call sites/tests that previously used the custom GetIterator() pattern.

Changes:

• Added IEnumerable<long> / IEnumerator<long> support to AbstractAppendingInt64Buffer via a new enumerator implementation.
• Updated DocValues writers to use the new enumerator instead of the removed iterator pattern.
• Updated tests and corrected private method naming typos (Enumberable → Enumerable) where touched.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/Lucene.Net/Util/Packed/AbstractAppendingLongBuffer.cs	Adds IEnumerable<long> support and replaces the custom iterator with an IEnumerator<long>-based enumerator.
src/Lucene.Net/Index/SortedSetDocValuesWriter.cs	Switches pending buffer iteration to use GetEnumerator() in iterator blocks.
src/Lucene.Net/Index/SortedDocValuesWriter.cs	Renames private enumerable helpers and switches ord iteration to GetEnumerator().
src/Lucene.Net/Index/NumericDocValuesWriter.cs	Switches numeric pending buffer iteration to GetEnumerator() in an iterator block.
src/Lucene.Net/Index/BinaryDocValuesWriter.cs	Switches lengths buffer iteration to GetEnumerator() in an iterator block.
src/Lucene.Net.Tests/Util/Packed/TestPackedInts.cs	Updates tests to validate enumeration via MoveNext()/Current.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[NightOwl888]

Well, this is an odd one. Upstream, the AbstractAppendingLongBuffer is package-private, not public. But, since .NET doesn't allow an abstract base class to be internal with public subclasses, this was marked public with a private protected constructor (meaning it cannot be inherited publicly).

I asked ChatGPT whether this means that users can see the iterator, and the answer was a bit odd. Yes, the iterator() method is available on the public API, but it returns type Object and since the iterator implementation is in a package-private class and Iterator<Long> was not implemented, there is no way to use it short of using Reflection (or at least there wasn't until Java 10). So, clearly this iterator wasn't intended for public use. ChatGPT further recommends that a port should simply mark the iterator/enumerator and factory method for it internal.

---

Then I researched a bit further, and it seems that in Lucene 4.10.0, all 4 of these "LongBuffer" types were removed and replaced with a new API in LUCENE-5792.

apache/lucene@a855131

Being that our goal is to stabilize the public API, the fact that these will be removed before even getting out of the 4.x version range is a problem, and we can add one more reason to the pile of reasons we should aim to upgrade to 4.10.4 before the release. Alternatively, we could stabilize the API at 4.8.1 (plus patches) and then move only to the latest 5.x version next. There is at least one reason why that would make sense - 5.x codecs were supported in the backward-codecs package until Lucene 9.x, making it possible to upgrade the software first and the index later. It isn't clear how difficult it would be to add 4.x index reading support to modern Lucene versions.

---

So, at the end of the day, there isn't much gained here. We are modifying APIs that are going away very soon. Furthermore, these APIs are not intended to be public.

Short term, it seems like keeping these changes except for the IEnumerable<long> interface and marking GetEnumerator() and Enumerator internal seems like the right move.

But if we are thinking longer term, perhaps we should make porting at least LUCENE-5792 a priority, which will avert an upcoming breaking API change.

Being that there is at least one more breaking API change coming in the 4.x branch (the CharsRef/BytesRef/Int32sRef/Int64sRef builders) we should either apply them before the 4.x release or wait until at least 5.x to introduce these breaking changes.

[paulirwin]

I agree that there isn't much gained here, but this has been an open issue forever and it finally clears this out. Also, I think there is some good gained here: the call sites are now cleaner.

(I'd like to take a second to emphasize that this issue has been open for over 14 years...)

Importantly, this PR also doesn't cause any problems. I think we can merge it as-is as a lucenenet-specific change.

As discussed in #1293 (comment), I do not believe we should move the goalposts and target 4.10.4. We're so close to the finish line, and our users are so desperate for a final release, that we can wrap up 4.8.1 (+ any important bug fixes only from 4.8.1-4.10.4), and then we can decide if we want to do a 4.10.4 small release (I think this would be a good idea) or pivot to a later release. But, I think we should discuss that there, not here.

Regardless, there is no harm in making this change, so I think we should do it and close out this issue that has been open for over a decade and call it done. Even if this code ends up going away soon.

[NightOwl888]

The real harm is that we are already breaking the public API and we will have to again when LUCENE-5702 is applied.

However, there is a happy medium where we can put this upgrade work off until after the release without doing a later breaking change - we can mark AbstractAppendingInt64Buffer and its 3 subclasses internal now. This still only breaks the API once. Then applying the changes in LUCENE-5702 later will be a public addition and fully backward compatible with Lucene.NET 4.8.0. I would only suggest merging this PR as-is if we can guarantee that the LUCENE-5702 change will happen before the 4.8.0 release.

The only downside is that this functionality will not be available to users until LUCENE-5702 is applied. However, being that this is very niche and users still have the option of copying this code out of the codebase if they need to use it, this doesn't seem like that big of an issue. At least they will be future proofed against breaking API changes.

[paulirwin]

@NightOwl888 agreed with making it internal as a middle-ground. The public subclasses were marked @lucene.internal already anyways signaling that they should not be for public use. Updated.

Edit: note that this is still technically a breaking change since we're making a public type internal, so I'm leaving the label as-is.

[paulirwin]

The real harm is that we are already breaking the public API and we will have to again when LUCENE-5702 is applied.

However, there is a happy medium where we can put this upgrade work off until after the release without doing a later breaking change - we can mark AbstractAppendingInt64Buffer and its 3 subclasses internal now. This still only breaks the API once. Then applying the changes in LUCENE-5702 later will be a public addition and fully backward compatible with Lucene.NET 4.8.0. I would only suggest merging this PR as-is if we can guarantee that the LUCENE-5702 change will happen before the 4.8.0 release.

The only downside is that this functionality will not be available to users until LUCENE-5702 is applied. However, being that this is very niche and users still have the option of copying this code out of the codebase if they need to use it, this doesn't seem like that big of an issue. At least they will be future proofed against breaking API changes.

One more thing... note that the JIRA number referenced in this comment is wrong. it should be LUCENE-5792 (as was mentioned in an earlier comment).