Skip to content

Rename PKLookupTask to BatchPKLookup (#555) - #604

Merged
mikemccand merged 1 commit into
mikemccand:mainfrom
serhiy-bzhezytskyy:rename-pklookup-batch
Jul 27, 2026
Merged

Rename PKLookupTask to BatchPKLookup (#555)#604
mikemccand merged 1 commit into
mikemccand:mainfrom
serhiy-bzhezytskyy:rename-pklookup-batch

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

Renames PKLookupTaskBatchPKLookup per #555, since it looks up N primary keys per task (the batch variant).

  • Renamed the class and its constructors, updated the call site in LocalTaskSource, and the javac classpaths in compile.sh and responseTimeTests.py.
  • Kept the reported category as "PKLookup" so nightly-chart history is unaffected, and left the historical annotations in notation.py as-is (they describe past events under the old name).
  • The Python side in benchUtil.py matches on the emitted TASK: PK… line and the PKLookup category, both unchanged, so it needs no changes.

Verified: src/main/perf/*.java compiles against a built Lucene 11.

I kept the sibling classes (PointsPKLookupTask, PKLookupWithTermStateTask) as-is; happy to rename those too for family consistency if you'd like.

PKLookupTask looks up N primary keys per task; the name didn't convey that
it's the batch variant (vs a single-PK lookup). Rename the class to
BatchPKLookup and update its call site in LocalTaskSource and the javac
classpaths in compile.sh and responseTimeTests.py.

The reported category stays 'PKLookup' so nightly-chart history is unaffected,
and the historical annotations in notation.py are left as-is (they describe
past events under the old name).
@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor Author

Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks

@mikemccand

Copy link
Copy Markdown
Owner

Thank you @serhiy-bzhezytskyy! +1 to rename the other two cases as long as they are also batch-optimized?

@mikemccand mikemccand left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks great, thank you @serhiy-bzhezytskyy! I'll merge shortly.

@mikemccand
mikemccand merged commit 9e32d22 into mikemccand:main Jul 27, 2026
1 check passed
@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor Author

Thanks for merging!

On renaming the other two — I checked, and they don't meet the condition, so I'd leave their names alone:

  • BatchPKLookup sorts the ids before looking them up, with the comment saying exactly why: // Sort so lookups are in term-order, optimizing for Lucene's fastest batch PK lookup use case. That's what earns the "Batch" in the name.
  • PKLookupWithTermStateTask does two passes over ids in generation order — first seekExact recording a TermState per segment, then re-seeking with that TermState to resolve docids. No sort, so no term-order locality; what it measures is the TermState caching, not batching.
  • PointsPKLookupTask builds an IntPoint.newExactQuery per id, also unsorted.

Neither has an Arrays.sort, so calling them "Batch…" would make the name assert an optimization the code doesn't do.

If you'd like them batched too, that's a small separate change — sort the ids in each, then rename. Worth noting it would change what those two tasks measure, so their numbers would shift and the history before/after wouldn't be comparable. I can put that up as its own PR if you think the parity is worth the discontinuity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants