fix some issues with filters on numeric columns with nulls #9251

clintropolis · 2020-01-24T09:19:23Z

Description

This PR fixes an issue with predicate filters, such as a bound filter, on long columns with numeric nulls, where it will incorrectly compare 0 instead of calling predicate.applyNull(). The correct code is actually in place already in LongValueMatcherColumnSelectorStrategy, but this was getting pre-empted by Filters.makeValueMatcher which had it's own predicate defined specifically handling long columns, along with a code comment:

// This should be folded into the ValueMatcherColumnSelectorStrategy once that can handle LONG typed columns.

Unfortunately, it just hadn't been done yet.

Prior to this PR, the added testLongPredicateFilterNulls in CalciteQueryTest would fail with an error of the form:

java.lang.AssertionError
	at org.apache.druid.segment.data.ColumnarLongs$1HistoricalLongColumnSelectorWithNulls.getLong(ColumnarLongs.java:124)
	at org.apache.druid.segment.filter.Filters$4.matches(Filters.java:467)
	at org.apache.druid.segment.FilteredOffset.increment(FilteredOffset.java:75)
	at org.apache.druid.segment.QueryableIndexCursorSequenceBuilder$QueryableIndexCursor.advance(QueryableIndexCursorSequenceBuilder.java:417)
	at org.apache.druid.query.timeseries.TimeseriesQueryEngine.lambda$processNonVectorized$2(TimeseriesQueryEngine.java:288)

While adding tests for this fix, I also encountered an issue with the JavascriptDimFilter, which did not implement applyNull for it's predicate, causing it to be unable to feed null values into javascript filter functions.

I also ran into some issues with vectorized value and predicate matchers on numeric null columns. The initial issue is that the matchers were not checking the null vector, however once that was in place I bumped into another issue that the null vector could be incorrectly polluted with the wrong null information in the case where the null bitmap ran out of values before the end of the column (likely) because it was breaking from the loop instead of writing false values until the end vector offset.

This PR has:

been self-reviewed.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

gianm · 2020-01-25T00:08:49Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

+                  .dataSource(CalciteTests.DATASOURCE3)
+                  .intervals(querySegmentSpec(Filtration.eternity()))
+                  .granularity(Granularities.ALL)
+                  .filters(bound("l1", "3", null, true, false, null, StringComparators.NUMERIC))


How about moving these three tests to BoundFilterTest? I suggest this for two reasons:

This test isn't adding much to coverage of the SQL layer.

BoundFilterTest extends from BaseFilterTest which tests a whole big matrix of configurations, so coverage of the lower level stuff will be better vs. using CalciteQueryTest.

Yeah, I'm working on adding some tests here to get the extra coverage

I did some refactoring around BaseFilterTest to add in numeric columns with null values, and bumped into some additional issues. First one in the JavascriptDimFilter, whose numeric predicates did not implement applyNull, meaning you couldn't specifically filter for the null value. The 'in' filter I believe also has this problem, assuming that something like .. x IN (1, 2, NULL) is valid, but is more involved to fix so I didn't do it in this PR (assuming it needs fixed?).

I also ran into some issues with vectorized value and predicate matchers on numeric null columns. The initial issue is that the matchers were not checking the null bitmap, however once that was in place I bumped into another issue that the null vector could be incorrectly polluted with the wrong null information in the case where the null bitmap ran out of values before the end of the column (likely) because it was breaking from the loop instead of writing false values until the end vector offset.

I added tests for all of these cases.

I did not dig into the expression filter for fear of what I might run into, but will investigate this as a follow-up to this PR, along with any remaining filters which I did not address.

gianm · 2020-01-27T23:17:56Z

processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java

@@ -57,7 +57,8 @@
        final int row = i + startOffset;
        nullIterator.advanceIfNeeded(row);
        if (!nullIterator.hasNext()) {
-          break;


How about using Arrays.fill to fill the rest with falses, and then break? Should be somewhat more efficient.

👍 will change

gianm · 2020-01-27T23:18:03Z

processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java

@@ -71,7 +72,8 @@
        final int row = currentOffsets[i];
        nullIterator.advanceIfNeeded(row);
        if (!nullIterator.hasNext()) {
-          break;


Same comment.

gianm · 2020-01-27T23:18:36Z

processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java

@@ -24,8 +24,20 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import junitparams.converters.Nullable;


Did you mean javax.annotation.Nullable?

Heh, oops, gg intellij

gianm

Another comment: did you consider removing the default impl of applyNull from the DruidXPredicate interfaces? It seems like it being missing is just asking for null handling bugs.

clintropolis · 2020-01-27T23:40:36Z

Another comment: did you consider removing the default impl of applyNull from the DruidXPredicate interfaces? It seems like it being missing is just asking for null handling bugs.

Yes, and I agree, though since I was planning to look at expression filters and the others as a follow-up I was considering making this change later. Should I just do it all now?

gianm · 2020-01-27T23:52:41Z

Yes, and I agree, though since I was planning to look at expression filters and the others as a follow-up I was considering making this change later. Should I just do it all now?

I think doing those as a follow-up is OK.

* fix issue with long column predicate filters and nulls * dang * uncomment a thing * styles * oops * allcaps * review stuff

fix issue with long column predicate filters and nulls

4bafeff

clintropolis added Bug Area - Querying Area - Null Handling labels Jan 24, 2020

clintropolis mentioned this pull request Jan 24, 2020

sql support for dynamic parameters #6974

Merged

gianm reviewed Jan 25, 2020

View reviewed changes

jon-wei mentioned this pull request Jan 25, 2020

Incorrect null handling for long columns in predicate matching filters #9255

Open

clintropolis added 5 commits January 25, 2020 17:20

dang

db5da99

uncomment a thing

c9a427e

styles

e84c0c7

oops

1d841e9

allcaps

67b5a08

clintropolis changed the title ~~fix issue with long column predicate matcher based filters and nulls~~ fix some issues with filters on numeric columns with nulls Jan 27, 2020

gianm reviewed Jan 27, 2020

View reviewed changes

review stuff

7c6bbb2

gianm approved these changes Jan 28, 2020

View reviewed changes

gianm merged commit 36c5efe into apache:master Jan 28, 2020

clintropolis deleted the predicate-filter-long-nulls branch January 28, 2020 02:54

jihoonson added this to the 0.18.0 milestone Mar 26, 2020

jihoonson mentioned this pull request Apr 19, 2020

[Draft] 0.18.0 release notes #9652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix some issues with filters on numeric columns with nulls #9251

fix some issues with filters on numeric columns with nulls #9251

clintropolis commented Jan 24, 2020 •

edited

Loading

gianm Jan 25, 2020

clintropolis Jan 25, 2020

gianm Jan 25, 2020

clintropolis Jan 27, 2020

gianm Jan 27, 2020

clintropolis Jan 27, 2020

gianm Jan 27, 2020

gianm Jan 27, 2020

clintropolis Jan 27, 2020

gianm left a comment

clintropolis commented Jan 27, 2020

gianm commented Jan 27, 2020

fix some issues with filters on numeric columns with nulls #9251

fix some issues with filters on numeric columns with nulls #9251

Conversation

clintropolis commented Jan 24, 2020 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

clintropolis commented Jan 27, 2020

gianm commented Jan 27, 2020

clintropolis commented Jan 24, 2020 •

edited

Loading