Skip to content

fix incorrect unnest dimension cursor value matcher implementation#15192

Merged
clintropolis merged 3 commits into
apache:masterfrom
clintropolis:fix-unnest-dimension-cursor-3vl
Oct 18, 2023
Merged

fix incorrect unnest dimension cursor value matcher implementation#15192
clintropolis merged 3 commits into
apache:masterfrom
clintropolis:fix-unnest-dimension-cursor-3vl

Conversation

@clintropolis

Copy link
Copy Markdown
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.

@soumyava soumyava left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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