On Fri, May 10, 2024 at 04:44:54PM -0500, Justin Tobler wrote:

On 24/05/08 01:04PM, Patrick Steinhardt wrote:

quoted

Reftable iterators are created by seeking on the parent structure of a
corresponding record. For example, to create an iterator for the merged
table you would call `reftable_merged_table_seek_ref()`. Most notably,
it is not posible to create an iterator and then seek it afterwards.

While this may be a bit easier to reason about, it comes with two
significant downsides. The first downside is that the logic to find
records is split up between the parent data structure and the iterator
itself. Conceptually, it is more straight forward if all that logic was
contained in a single place, which should be the iterator.

The second and more significant downside is that it is impossible to
reuse iterators for multiple seeks. Whenever you want to look up a
record, you need to re-create the whole infrastructure again, which is
quite a waste of time. Furthermore, it is impossible to for example
optimize seeks, for example when seeking the same record multiple times.

The last setence could use some rewording.

"Furthermore, it is impossible to optimize seeks, such as when seeking
the same record multiple times."

Done.

[snip]

quoted

diff --git a/reftable/generic.c b/reftable/generic.c
index b9f1c7c18a..1cf68fe124 100644
--- a/reftable/generic.c
+++ b/reftable/generic.c

@@ -12,25 +12,39 @@ license that can be found in the LICENSE file or at
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
+void table_init_iter(struct reftable_table *tab,

The following table related functions are prefixed with `reftable_`. Do
we want to do the same here?

Functions with the `reftable_` prefix are supposed to be public, whereas
functions without them are private. So this is intentionally missing the
prefix.

[snip]

quoted

@@ -23,6 +23,13 @@ static void filtering_ref_iterator_close(void *iter_arg)
 	reftable_iterator_destroy(&fri->it);
 }
 
+static int filtering_ref_iterator_seek(void *iter_arg,
+				       struct reftable_record *want)
+{
+	struct filtering_ref_iterator *fri = iter_arg;
+	return iterator_seek(&fri->it, want);
+}

I've found the `filtering_ref_iterator_seek()` here to be a little
confusing. At first, I assumed that the `filtering_ref_iterator` would
have referenced `filtering_ref_iterator_vtable` thus resulting in a
cycle, but on closer inspection this does not seem to be the case and is
in face always set to some other iterator operation.

Am I understanding this correctly?

Yes. The filtering ref iterator wraps a _different_ iterator, which is
`fri->it` in the above case, and only returns a subset of the records of
that wrapped iterator. So we eventually end up calling the callbacks of
the wrapped iterator, which are likely not a filtering ref iterator
themselves (even though that would in theory be possible).

Patrick