[v2,03/13] reftable/reader: unify indexed and linear seeking

Commit Message

Patrick Steinhardt May 13, 2024, 8:47 a.m. UTC

In `reader_seek_internal()` we either end up doing an indexed seek when
there is one or a linear seek otherwise. These two code paths are
disjunct without a good reason, where the indexed seek will cause us to
exit early.

Refactor the two code paths such that it becomes possible to share a bit
more code between them.

Signed-off-by: Patrick Steinhardt <[e-mail]>
---
 reftable/reader.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

Comments

Karthik Nayak May 21, 2024, 2:41 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> In `reader_seek_internal()` we either end up doing an indexed seek when
> there is one or a linear seek otherwise. These two code paths are

Are we missing the subject here?

[snip]

Patrick Steinhardt May 22, 2024, 7:23 a.m. UTC | #2

On Tue, May 21, 2024 at 02:41:27PM +0000, Karthik Nayak wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> > In `reader_seek_internal()` we either end up doing an indexed seek when
> > there is one or a linear seek otherwise. These two code paths are
> 
> Are we missing the subject here?

Hm. "one" refers back to "indexed" and is supposed to mean "index" here.
I agree that this is easy to misread though and may not even be proper
English.

It doesn't really feel worth it to rebase this series just for this
issue, but if I did it would be 's/one/an index/'. Let me know in case
you disagree though.

Patrick

Karthik Nayak May 22, 2024, 7:56 a.m. UTC | #3

Patrick Steinhardt <[e-mail]> writes:

> On Tue, May 21, 2024 at 02:41:27PM +0000, Karthik Nayak wrote:
>> Patrick Steinhardt <[e-mail]> writes:
>>
>> > In `reader_seek_internal()` we either end up doing an indexed seek when
>> > there is one or a linear seek otherwise. These two code paths are
>>
>> Are we missing the subject here?
>
> Hm. "one" refers back to "indexed" and is supposed to mean "index" here.
> I agree that this is easy to misread though and may not even be proper
> English.
>
> It doesn't really feel worth it to rebase this series just for this
> issue, but if I did it would be 's/one/an index/'. Let me know in case
> you disagree though.
>
> Patrick

Okay that makes a bunch of sense, I think it is okay though and not
worth a reroll.

Karthik

Patch

diff --git a/reftable/reader.c b/reftable/reader.c
index 6bfadcad71..cf7f126d8d 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -425,7 +425,7 @@  static int reader_seek_linear(struct table_iter *ti,
 	struct strbuf want_key = STRBUF_INIT;
 	struct strbuf got_key = STRBUF_INIT;
 	struct reftable_record rec;
-	int err = -1;
+	int err;
 
 	reftable_record_init(&rec, reftable_record_type(want));
 	reftable_record_key(want, &want_key);
@@ -499,8 +499,8 @@  static int reader_seek_linear(struct table_iter *ti,
 	return err;
 }
 
-static int reader_seek_indexed(struct reftable_reader *r,
-			       struct reftable_iterator *it,
+static int reader_seek_indexed(struct table_iter *ti,
+			       struct reftable_reader *r,
 			       struct reftable_record *rec)
 {
 	struct reftable_record want_index = {
@@ -510,13 +510,9 @@  static int reader_seek_indexed(struct reftable_reader *r,
 		.type = BLOCK_TYPE_INDEX,
 		.u.idx = { .last_key = STRBUF_INIT },
 	};
-	struct table_iter ti = TABLE_ITER_INIT, *malloced;
-	int err = 0;
+	int err;
 
 	reftable_record_key(rec, &want_index.u.idx.last_key);
-	err = reader_start(r, &ti, reftable_record_type(rec), 1);
-	if (err < 0)
-		goto done;
 
 	/*
 	 * The index may consist of multiple levels, where each level may have
@@ -524,7 +520,7 @@  static int reader_seek_indexed(struct reftable_reader *r,
 	 * highest layer that identifies the relevant index block as well as
 	 * the record inside that block that corresponds to our wanted key.
 	 */
-	err = reader_seek_linear(&ti, &want_index);
+	err = reader_seek_linear(ti, &want_index);
 	if (err < 0)
 		goto done;
 
@@ -550,36 +546,30 @@  static int reader_seek_indexed(struct reftable_reader *r,
 		 * all levels of the index only to find out that the key does
 		 * not exist.
 		 */
-		err = table_iter_next(&ti, &index_result);
+		err = table_iter_next(ti, &index_result);
 		if (err != 0)
 			goto done;
 
-		err = reader_table_iter_at(r, &ti, index_result.u.idx.offset, 0);
+		err = reader_table_iter_at(r, ti, index_result.u.idx.offset, 0);
 		if (err != 0)
 			goto done;
 
-		err = block_iter_seek_key(&ti.bi, &ti.br, &want_index.u.idx.last_key);
+		err = block_iter_seek_key(&ti->bi, &ti->br, &want_index.u.idx.last_key);
 		if (err < 0)
 			goto done;
 
-		if (ti.typ == reftable_record_type(rec)) {
+		if (ti->typ == reftable_record_type(rec)) {
 			err = 0;
 			break;
 		}
 
-		if (ti.typ != BLOCK_TYPE_INDEX) {
+		if (ti->typ != BLOCK_TYPE_INDEX) {
 			err = REFTABLE_FORMAT_ERROR;
 			goto done;
 		}
 	}
 
-	REFTABLE_ALLOC_ARRAY(malloced, 1);
-	*malloced = ti;
-	iterator_from_table_iter(it, malloced);
-
 done:
-	if (err)
-		table_iter_close(&ti);
 	reftable_record_release(&want_index);
 	reftable_record_release(&index_result);
 	return err;
@@ -595,15 +585,15 @@  static int reader_seek_internal(struct reftable_reader *r,
 	struct table_iter ti = TABLE_ITER_INIT, *p;
 	int err;
 
-	if (idx > 0)
-		return reader_seek_indexed(r, it, rec);
-
-	err = reader_start(r, &ti, reftable_record_type(rec), 0);
+	err = reader_start(r, &ti, reftable_record_type(rec), !!idx);
 	if (err < 0)
 		goto out;
 
-	err = reader_seek_linear(&ti, rec);
-	if (err < 0)
+	if (idx)
+		err = reader_seek_indexed(&ti, r, rec);
+	else
+		err = reader_seek_linear(&ti, rec);
+	if (err)
 		goto out;
 
 	REFTABLE_ALLOC_ARRAY(p, 1);