Skip to content

fix: EmptyCursorHolder#canVectorize must depend on the CursorBuildSpec. - #20822

Merged
gianm merged 1 commit into
apache:masterfrom
gianm:fix-vec-empty-holder
Nov 3, 2026
Merged

gianm merged 1 commit into
apache:masterfrom
gianm:fix-vec-empty-holder

Conversation

@gianm

@gianm gianm commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Previously, EmptyCursorHolder#canVectorize always returned true. However, the expectation for CursorHolders is that they should check the virtual columns and aggregators from the spec. Not doing so led to the vector path being taken for virtual columns and aggregators that are not able to vectorize.

Previously, EmptyCursorHolder#canVectorize always returned true. However,
the expectation for CursorHolders is that they should check the
virtual columns and aggregators from the spec. Not doing so led to
the vector path being taken for virtual columns and aggregators that
are not able to vectorize.

@clintropolis clintropolis 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.

Does it make a difference having a real inspector vs just always using an 'empty' signature?

@gianm

gianm commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Does it make a difference having a real inspector vs just always using an 'empty' signature?

I think some things can decide not to vectorize if input types aren't known. One example is FallbackVectorProcessor which requires a known outputType, and the output type might be unknown (i.e. null) if the input type is unknown. I'm not sure if this is "correct" -- maybe it should allow vectorization of unknown types? -- but it is a thing currently.

@gianm
gianm merged commit df71642 into apache:master Nov 3, 2026
27 checks passed
@github-actions github-actions Bot added this to the 40.0.0 milestone Nov 3, 2026
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.

2 participants