Skip to content

feat: unnest and join makeCursorHolderAsync#19600

Open
clintropolis wants to merge 1 commit into
apache:masterfrom
clintropolis:async-unnest-and-join
Open

feat: unnest and join makeCursorHolderAsync#19600
clintropolis wants to merge 1 commit into
apache:masterfrom
clintropolis:async-unnest-and-join

Conversation

@clintropolis

@clintropolis clintropolis commented Jun 19, 2026

Copy link
Copy Markdown
Member

Description

This PR adds makeCursorHolderAsync implementations to UnnestCursorFactory and HashJoinSegmentCursorFactory, and also switches the default implementation of makeCursorHolderAsync to explode with a defensive exception instead of call the sync method. A new ResidentCursorFactory interface 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.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we want to make implementations of CursorFactory implement this method, why offer a default at all?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for the context!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants