Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dictionary handling in AggregationMaskCompiler #21363

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

dain
Copy link
Member

@dain dain commented Apr 2, 2024

Description

Generated AggregationMask was not properly handling dictionary encoded mask blocks.

Fixes #21272

Release notes

(x) Release notes are required, with the following suggested text:

# General
* Fix `IllegalArgumentException` in generated aggregation mask. ({issue}`21272`)

@cla-bot cla-bot bot added the cla-signed label Apr 2, 2024
@dain dain requested review from electrum and martint April 2, 2024 18:50
@wendigo
Copy link
Contributor

wendigo commented Apr 2, 2024

Can we add a test to avoid future regressions?

Variable position = scope.declareVariable("position", body, constantInt(0));
BytecodeExpression isPositionSelected = testMaskBlock(maskValueBlock, maskBlockMayHaveNull, position);
Variable maskValueBlockPosition = scope.declareVariable("maskValueBlockPosition", body, constantInt(0));
BytecodeExpression isMaskPositionSelected = testMaskBlock(maskValueBlock, maskBlockMayHaveNull, maskValueBlockPosition);
Copy link
Member

Choose a reason for hiding this comment

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

inline?

@findepi
Copy link
Member

findepi commented Apr 3, 2024

Can we add a test to avoid future regressions?

there are repro steps in #21272 (comment).
they worked for me

@wendigo
Copy link
Contributor

wendigo commented Apr 3, 2024

@findepi test cases are not part of this change

@dain
Copy link
Member Author

dain commented Apr 3, 2024

there are repro steps in #21272 (comment). they worked for me

Reproducing this requires a specific organization of blocks and dictionaries, so using a query to do this is pretty brittle. To really test this the aggregation mask must be driven manally with the correct combination of blocks. I might be able to look at adding a test like that today, but it will likely be tomorrow or Friday.

Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

Let's get this in so that we can release today.

We can follow up with the test later.

@martint martint merged commit 8681800 into trinodb:master Apr 3, 2024
95 checks passed
@github-actions github-actions bot added this to the 444 milestone Apr 3, 2024
@dain dain deleted the fix-aggregation-mask-builder branch April 3, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

AggregationMask generated code throws IllegalArgumentException: Invalid position %d in block with %d positions
5 participants