Re: [PATCH 07/13] reftable/merged: split up initialization and seeking of records
- From
Justin Tobler <jltobler@gmail.com>
- Date
- May 10, 2024, 19:18 UTC
- Message-ID
- <dqnhhjdingkhhwwp5nwut2hsnkkze7m2ls4kscddqxm3txjzbf@5knvdk4vc6eg>
- In-Reply-To
- <21b3e3ab5f04e66fdd352187b1da699d1ab67cee.1715166175.git.ps@pks.im>
On 24/05/08 01:04PM, Patrick Steinhardt wrote:
> To initialize a `struct merged_iter`, we need to seek all subiterators > to the wanted record and then add their results to the priority queue > used to sort the records. This logic is split up across two functions, > `merged_table_seek_record()` and `merged_table_iter()`. The scope of
Did we mean `merged_iter_init` instead of `merged_table_iter()` here?
Show 12 quoted lines
> these functions is somewhat weird though, where `merged_table_iter()` is > only responsible for adding the records of the subiterators to the > priority queue. > > Clarify the scope of those functions such that `merged_table_iter()` is > only responsible for initializing the iterator's structure. Performing > the subiterator seeks are now part of `merged_table_seek_record()`. > > This step is required to move seeking of records into the generic > `struct reftable_iterator` infrastructure. > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
...
Show 20 quoted lines
> @@ -246,32 +230,33 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,
> struct reftable_iterator *it,
> struct reftable_record *rec)
> {
> - struct merged_iter merged = {
> - .typ = reftable_record_type(rec),
> - .hash_id = mt->hash_id,
> - .suppress_deletions = mt->suppress_deletions,
> - .advance_index = -1,
> - };
> - struct merged_iter *p;
> + struct merged_iter merged, *p;
> int err;
>
> - REFTABLE_CALLOC_ARRAY(merged.subiters, mt->stack_len);
> + merged_iter_init(&merged, mt);
> +
> for (size_t i = 0; i < mt->stack_len; i++) {
> + reftable_record_init(&merged.subiters[merged.stack_len].rec,
> + reftable_record_type(rec));I find it somewhat confusing how we increment the subiterator index here. I assume this is because when a record is not found we don't want to add it to the index. Might be nice to add a comment here explaining this.
Edit: Looks like you address this in the next commit so I guess we are good :)
Show 32 quoted lines
> + > err = reftable_table_seek_record(&mt->stack[i], > &merged.subiters[merged.stack_len].iter, rec); > if (err < 0) > goto out; > - if (!err) > - merged.stack_len++; > - } > + if (err > 0) > + continue; > > - err = merged_iter_init(&merged); > - if (err < 0) > - goto out; > + err = merged_iter_advance_subiter(&merged, merged.stack_len); > + if (err < 0) > + goto out; > + > + merged.stack_len++; > + } > > - p = reftable_malloc(sizeof(struct merged_iter)); > + p = reftable_malloc(sizeof(*p)); > *p = merged; > iterator_from_merged_iter(it, p); > + err = 0; > > out: > if (err < 0) > -- > 2.45.0 >