Re: [PATCH 09/13] reftable/generic: move seeking of records into the iterator

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]

On Fri, May 10, 2024 at 04:44:54PM -0500, Justin Tobler wrote:
> On 24/05/08 01:04PM, Patrick Steinhardt wrote:
> > 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]
> > 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]
> > @@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]