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 incorrect unnest dimension cursor value matcher implementation #15192

Merged

Conversation

clintropolis
Copy link
Member

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 whenever includeUnknown 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 when includeUnknown is true, and so instead would effectively match nothing when used with a not filter. Added tests cover the case.
This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

Copy link
Contributor

@soumyava soumyava left a 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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member Author

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

@clintropolis clintropolis merged commit 5c14b42 into apache:master Oct 18, 2023
85 checks passed
@clintropolis clintropolis deleted the fix-unnest-dimension-cursor-3vl branch October 18, 2023 23:43
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