Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement iterator deduplicator #3381

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

Factored out of #3379.

How this works

While looking for the Next value - if the value was already seen, skip it. If the value wasn't seen, mark it as seen and stop iterating.

How this was tested

  • Added unit test

@StephenButtolph StephenButtolph self-assigned this Sep 11, 2024
@StephenButtolph StephenButtolph added this to the v1.11.12 milestone Sep 11, 2024

var _ Iterator[any] = (*deduplicator[any])(nil)

type deduplicator[T comparable] struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type deduplicator[T comparable] struct {
	Iterator[T]
	seen set.Set[T]
}

Wondering if it makes sense to just embed the iterator and drop Value and Release

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I works locally for me:

index b36c1e0a1..4ba769c12 100644
--- a/utils/iterator/deduplicate.go
+++ b/utils/iterator/deduplicate.go
@@ -8,7 +8,7 @@ import "github.com/ava-labs/avalanchego/utils/set"
 var _ Iterator[any] = (*deduplicator[any])(nil)
 
 type deduplicator[T comparable] struct {
-       it   Iterator[T]
+       Iterator[T]
        seen set.Set[T]
 }
 
@@ -16,13 +16,13 @@ type deduplicator[T comparable] struct {
 // been returned from [it].
 func Deduplicate[T comparable](it Iterator[T]) Iterator[T] {
        return &deduplicator[T]{
-               it: it,
+               Iterator: it,
        }
 }
 
 func (i *deduplicator[_]) Next() bool {
-       for i.it.Next() {
-               element := i.it.Value()
+       for i.Iterator.Next() {
+               element := i.Iterator.Value()
                if !i.seen.Contains(element) {
                        i.seen.Add(element)
                        return true
@@ -30,11 +30,3 @@ func (i *deduplicator[_]) Next() bool {
        }
        return false
 }
-
-func (i *deduplicator[T]) Value() T {
-       return i.it.Value()
-}
-
-func (i *deduplicator[_]) Release() {
-       i.it.Release()
-}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this custom iterator implementation and reused Filter.

Copy link
Contributor Author

@StephenButtolph StephenButtolph Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embedding does work - but it has introduced subtle bugs in the past (because changing the signature of Next to NewNext or something wouldn't cause the code to fail to compile, but would cause the implementation to be wrong)...

This is why we use Unsafe.*Server rather than Unimplemented.*Server in our grpc services.

Side note - calling it Unsafe was ridiculous. I'll never understand decisions like that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminds me of InsecureSkipVerify

@StephenButtolph StephenButtolph added this pull request to the merge queue Sep 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 11, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue Sep 11, 2024
Merged via the queue into master with commit 878a6ce Sep 11, 2024
20 of 21 checks passed
@StephenButtolph StephenButtolph deleted the implement-deduplication-iterator branch September 11, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants