-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 incorrect unnest dimension cursor value matcher implementation #15192
fix incorrect unnest dimension cursor value matcher implementation #15192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Minor nits
.addOrderByColumn("alias0", OrderByColumnSpec.Direction.ASCENDING) | ||
.build(); | ||
|
||
// Total rows should add up to 26 * 2 = 52 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. this comment is not relevant here
.addOrderByColumn("alias0", OrderByColumnSpec.Direction.ASCENDING) | ||
.build(); | ||
|
||
// Total rows should add up to 26 * 2 = 52 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. there are 52 values as filter is on non existent value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, i copied this whole test from another test and just added filters, will just remove
Description
Fixes an issue from #15058 with value matchers created by
UnnestDimensionCursor
for matches against non-existent values, which I forgot to update to match only null rows wheneverincludeUnknown
is set to true. I checked around and didn't notice any other implementations with this issue, the problem was that it was treating the case as a non-existent column instead of an always false matcher that should only match null rows whenincludeUnknown
is true, and so instead would effectively match nothing when used with a not filter. Added tests cover the case.This PR has: