More consistent empty-set filtering behavior on multi-value columns.

[gianm]

The behavior is now that filters on "null" will match rows with no
values. The behavior in the past was inconsistent; sometimes these
filters would match and sometimes they wouldn't.

Adds tests for this behavior to SelectorFilterTest and
BoundFilterTest, for query-level filters and filtered aggregates.

Fixes #2750.

The big change here is that IndexMerger includes empty rows in
its inverted index for the null value. IndexMergerV9 already did
this so it's not being changed. IncrementalIndex already counted
empty inputs as matching a filter for null, so it's not being
changed either.

[jon-wei]

👍

[binlijin]

I have a question, when convert v8 to v9, why not have to calculate nullsSet with multi-value columns.

public class IndexIO
{
    public void convertV8toV9(File v8Dir, File v9Dir, IndexSpec indexSpec)
        throws IOException
    {
          ...
          for (int i = 0; i < multiValCol.size(); ++i) {
            VSizeIndexedInts rowValue = multiValCol.get(i);
            if (!onlyOneValue) {
              break;
            }
            if (rowValue.size() > 1) {
              onlyOneValue = false;
            }
            if (rowValue.size() == 0 || rowValue.get(0) == emptyStrIdx) {
              if (nullsSet == null) {
                nullsSet = bitmapFactory.makeEmptyMutableBitmap();
              }
              nullsSet.add(i);
            }
          }
          ...
    }
}

[gianm]

Would appreciate it if anyone who cares about null handling could take a look.

[gianm]

@binlijin in convertV8toV9, nullsSet is only used for conversion of multi-value columns to single-value columns; so it's not used if a column is going to remain multi-value.

[binlijin]

Thank you very much @gianm.

[fjy]

👍

[gianm]

added docs to the PR clarifying behavior of multi-value filtering.

[himanshug]

add(..) on MutableBitmap is expensive and leads to a performance regression here because this will, in many cases, get called for num_rows * num_dimensions

[gianm]

I got this code from IndexMergerV9 so it is probably an issue there too.

Although now I wonder if this check is really necessary. nullRowsList is just used to add empty rows to the inverted index for null. Those rows already containing explicit nulls should already be in the inverted index – so this might be duplicating work?

[gianm]

I removed this and a bunch of filter tests failed so it does seem like it's doing something.

[jon-wei]

I think in IndexMergerV9, nullRowsList was analogous to this part of IndexIO, in convertV8toV9():
https://github.com/druid-io/druid/blob/master/processing/src/main/java/io/druid/segment/IndexIO.java#L621

[jon-wei]

Looked into this a bit, I think this "null bitmap" code in IndexMerger/IndexMergerV9 is handling two things:

• IncrementalIndexAdapter doesn't build bitmap indexes for null fully, it doesn't take into account empty or missing rows:
  jon-wei@59e1090. You can see that if you disable all of the "null bitmap" logic and run IndexMergerTest.testPersistWithDifferentDims()
• The "null bitmap" code also handles cases where two segments are merged and a dimension was only present in one, you can see that behavior by disable the "null bitmap" code and running IndexMergerTest.testDisjointDimMerge()

If I disable the code pointed to by this comment thread in IndexMerger, I think some of the recently introduced filter tests on multival dimensions fails; enabling the "null bitmap" adjustment code in IndexIO.convertV8toV9() for multivalued dimensions as well as single valued dimensions seems to have the same effect as this change in question (but I haven't tested this extensively, only ran SelectorFilterTest)

[jon-wei]

So I think this code may be redundant for the single value dimension case