Skip to content

Fix vectorized cardinality bug on certain string columns.#11199

Merged
gianm merged 3 commits intoapache:masterfrom
gianm:fix-cardinality-vector-aggregator
May 7, 2021
Merged

Fix vectorized cardinality bug on certain string columns.#11199
gianm merged 3 commits intoapache:masterfrom
gianm:fix-cardinality-vector-aggregator

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented May 5, 2021

Fixes a bug introduced in #11182, related to the fact that in some cases,
ColumnProcessors.makeVectorProcessor will call "makeObjectProcessor"
instead of "makeSingleValueDimensionProcessor" or
"makeMultiValueDimensionProcessor". CardinalityVectorProcessorFactory
improperly ignored calls to "makeObjectProcessor".

In addition to fixing the bug, I added this detail to the javadocs for
VectorColumnProcessorFactory, to prevent others from running into the
same thing in the future. They do not currently call out this case.

Fixes a bug introduced in apache#11182, related to the fact that in some cases,
ColumnProcessors.makeVectorProcessor will call "makeObjectProcessor"
instead of "makeSingleValueDimensionProcessor" or
"makeMultiValueDimensionProcessor". CardinalityVectorProcessorFactory
improperly ignored calls to "makeObjectProcessor".

In addition to fixing the bug, I added this detail to the javadocs for
VectorColumnProcessorFactory, to prevent others from running into the
same thing in the future. They do not currently call out this case.
Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

👍

{
/**
* Called when {@link ColumnCapabilities#getType()} is STRING and the underlying column always has a single value
* Called only if {@link ColumnCapabilities#getType()} is STRING and the underlying column always has a single value
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.

woops, I guess I only fixed up docs for VectorColumnSelectorFactory in #10613, should've done these too, my bad

@gianm gianm merged commit a1f850d into apache:master May 7, 2021
@gianm gianm deleted the fix-cardinality-vector-aggregator branch May 7, 2021 15:37
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
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