feat: unnest and join makeCursorHolderAsync#19600
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 13 of 13 changed files.
This is an automated review by Codex GPT-5.5
There was a problem hiding this comment.
if we want to make implementations of CursorFactory implement this method, why offer a default at all?
There was a problem hiding this comment.
CursorFactory is sort indirectly a @PublicApi via Segment#as and also a sort of very low level extension point for custom segment implementations, so adding a new method with no default means extension writers would need to implement it and recompile for functionality that is currently at least off by default, so it seemed nicer to implement it so that they don't need to recompile immediately, making this async stuff more opt-in. I was thinking if something changes that makes it so that this path can be hit by default then I would change this to remove the default because at that point it really would need to be implemented
There was a problem hiding this comment.
Makes sense, thanks for the context!
Description
This PR adds
makeCursorHolderAsyncimplementations toUnnestCursorFactoryandHashJoinSegmentCursorFactory, and also switches the default implementation ofmakeCursorHolderAsyncto explode with a defensive exception instead of call the sync method. A newResidentCursorFactoryinterface has been added that can be used for any factory implementations that never have a reason to wait on stuff to avoid having to copy a default implementation everywhere.