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.

Sign up for GitHub

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

Jump to bottom

add native filters for "(filter) is true" and "(filter) is false" #15182

Merged
merged 5 commits into from
Oct 18, 2023
Merged

add native filters for "(filter) is true" and "(filter) is false" #15182

merged 5 commits into from
Oct 18, 2023

Conversation

clintropolis
Copy link
Member

Description

changes:

  • add IsTrueDimFilter, IsFalseDimFilter, and abstract IsBooleanDimFilter for native json filter implementations of (filter) IS TRUE and (filter) IS FALSE
  • add IsBooleanFilter for actual filtering logic for these filters, which ignore includeUnknown to always use matches with false for true and !matches with true for false
  • fix test incorrectly adjusted to wrong answer in sql compatible three-valued logic native filters  #15058

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • 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.

@clintropolis
add native filters for "(filter) is true" and "(filter) is false"
6d49c2a 
changes:
* add IsTrueDimFilter, IsFalseDimFilter, and abstract IsBooleanDimFilter for native json filter implementations of `(filter) IS TRUE` and `(filter) IS FALSE`
* add IsBooleanFilter for actual filtering logic for these filters, which ignore includeUnknown to always use matches with false for true and !matches with true for false
* fix test incorrectly adjusted to wrong answer in apache#15058
@clintropolis clintropolis added this to the 28.0 milestone Oct 17, 2023
soumyava
soumyava reviewed Oct 18, 2023
View reviewed changes
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.

Took 1 pass, looks good to me so far. Will take another pass tomorrow

processing/src/main/java/org/apache/druid/query/filter/IsBooleanDimFilter.java
)
{
if (field == null) {
throw DruidException.forPersona(DruidException.Persona.USER)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe replace with InvalidInput.exception()

processing/src/test/java/org/apache/druid/segment/filter/EqualityFilterTests.java
IsFalseDimFilter.of(new EqualityFilter("s0", ColumnType.STRING, "a", null)),
ImmutableList.of("0", "2", "4")
);
// "(s0 = 'a') is not false", same rows as "s0 = 'a'", but also with null rows
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments are great !

sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java
return toSimpleLeafFilter(
plannerContext,
rowSignature,
virtualColumnRegistry,
Iterables.getOnlyElement(((RexCall) rexNode).getOperands())
);
} else if (kind == SqlKind.IS_FALSE || kind == SqlKind.IS_NOT_TRUE) {
if (NullHandling.useThreeValueLogic()) {
// use expression filter to get isfalse or nottrue expressions for correct 3vl behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. isnottrue in place of nottrue

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, i was being consistent with the native expression names:

soumyava
soumyava approved these changes Oct 18, 2023
View reviewed changes
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.

Took a 2nd pass. Checked it out and ran some test cases around it with nullif. LGTM !

@clintropolis clintropolis merged commit 061cfee into apache:master Oct 18, 2023
81 checks passed
@clintropolis clintropolis deleted the 3vl-is-true-is-false branch October 18, 2023 20:07
@clintropolis clintropolis mentioned this pull request Oct 18, 2023
Merged
clintropolis added a commit to clintropolis/druid that referenced this pull request Oct 18, 2023
@clintropolis
add native filters for "(filter) is true" and "(filter) is false" (ap…
1bf5b48 
…ache#15182)

* add native filters for "(filter) is true" and "(filter) is false"

changes:
* add IsTrueDimFilter, IsFalseDimFilter, and abstract IsBooleanDimFilter for native json filter implementations of `(filter) IS TRUE` and `(filter) IS FALSE`
* add IsBooleanFilter for actual filtering logic for these filters, which ignore includeUnknown to always use matches with false for true and !matches with true for false
* fix test incorrectly adjusted to wrong answer in apache#15058
* add tests for default value mode
LakshSingla pushed a commit that referenced this pull request Oct 19, 2023
@clintropolis
add native filters for "(filter) is true" and "(filter) is false" (#1…
0825251 
…5182) (#15197)

* add native filters for "(filter) is true" and "(filter) is false"

changes:
* add IsTrueDimFilter, IsFalseDimFilter, and abstract IsBooleanDimFilter for native json filter implementations of `(filter) IS TRUE` and `(filter) IS FALSE`
* add IsBooleanFilter for actual filtering logic for these filters, which ignore includeUnknown to always use matches with false for true and !matches with true for false
* fix test incorrectly adjusted to wrong answer in #15058
* add tests for default value mode
@clintropolis clintropolis mentioned this pull request Oct 19, 2023
Merged
CaseyPan pushed a commit to CaseyPan/druid that referenced this pull request Nov 17, 2023
@clintropolis
add native filters for "(filter) is true" and "(filter) is false" (ap…
a02d07b 
…ache#15182)

* add native filters for "(filter) is true" and "(filter) is false"

changes:
* add IsTrueDimFilter, IsFalseDimFilter, and abstract IsBooleanDimFilter for native json filter implementations of `(filter) IS TRUE` and `(filter) IS FALSE`
* add IsBooleanFilter for actual filtering logic for these filters, which ignore includeUnknown to always use matches with false for true and !matches with true for false
* fix test incorrectly adjusted to wrong answer in apache#15058
* add tests for default value mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soumyava soumyava soumyava approved these changes

@pranavbhole pranavbhole pranavbhole approved these changes

Assignees
No one assigned
Labels
Area - Null Handling Area - Querying Area - Segment Format and Ser/De Area - SQL Bug
Projects
None yet
Milestone
28.0
Development

Successfully merging this pull request may close these issues.
3 participants
@clintropolis @pranavbhole @soumyava