Comparing dimensions to each other using a select filter

[erikdubbelboer]

See #3840 for a discussion on the design choices.

Most of the pull request is adding the new argument to SelectorDimFilter. Since SelectorDimFilter has an @JsonCreator constructor we can not add an extra constructor with the extra argument (only one @JsonCreator allowed).

Since all current valueMatcher code is build around matching a fixed value against one column, I had to add a new method to
ValueMatcherColumnSelectorStrategy: ValueGetter makeValueGetter(ValueSelectorType selector). This is then used in the new DimensionsFilter to get all values and compare them to each other.

[erikdubbelboer]

I'm thinking now that I could add the old constructor to SelectorDimFilter but just not annotate it with @JsonCreator. That way the diff would be a lot smaller as all current invocations could use this old constructor. Would this work and would this be better?

Is there a test somewhere that tests this JSON parsing?

[erikdubbelboer]

I cleaned up the changes a lot by adding the extra constructor to SelectorDimFilter.

Old changes can still be found at: erikdubbelboer@81e1c1f

[gianm]

Didn't fully review, but have some partial comments for now. Will pick back up again.

[gianm]

Please convert whitespace to spaces instead of tabs.

[gianm]

Just have this call this(dimension, value, extractionFn, null) so the checks/translations in the constructor only have to be in one place.

[gianm]

Please check another two things:

1. value should be null if dimensions is set (someone that sets both is probably confused about usage)
2. dimension and dimensions should not both be set

[gianm]

EqualColumnsFilter is a better name

[gianm]

throw UnsupportedOperationException

[gianm]

This String[], and the ValueGetter itself, could both be extracted to constants.

[gianm]

It's too bad (for performance) that this means that comparing two integer columns against each other will involve casting both of them to string. I'm ok with developing a way to deal with that in a future patch, but please leave a comment that this is less than ideal.

[erikdubbelboer]

After this pull requests is merged it's a good idea to fix this. But right now it would result in a lot more code making the pull requests even harder to comprehend.

I think the only way to do this is in makeMatcher to see if all columns are integer or float. And then call a different makeValueMatcher that compares integers/floats instead of strings. This would mean the code of makeValueMatcher and overlap needs to be duplicated twice for integers and floats. That's another 106 lines of code in this file.

[gianm]

We do similar things in other areas where we need specializations for performance reasons, so I think that's just a cost of doing business in a language like java. For now the comment is good enough for me.

[gianm]

List<String> is more typical although I guess it doesn't really matter

[erikdubbelboer]

I don't think it really matters as it's a fixed list. If you want me to change it let me know.

[gianm]

Nah it's fine the way it is.

[erikdubbelboer]

I'm pretty sure the test timeout has nothing to do with my code.

[jon-wei]

@erikdubbelboer it's an issue with the tests that comes up sometimes, I just closed/re-opened to re-run the tests

[jon-wei]

What do you think about changing this filter such that it accepts a list of DimensionSpec instead, which would allow the user to specify a separate extraction function for each dimension in the comparison?

I can see various use cases where that functionality would be helpful (e.g., extracting and comparing a substring from two columns, but where the two column's base values have totally different formats causing a shared extractionFn to be insufficient)

The DimensionSpecs could also be directly passed to DimensionHandlerUtils.createColumnSelectorPlus, so that this filter doesn't need to manage the extraction functions itself in the matcher.

---

Related to that, I think it makes sense to have EqualColumnsFilter be a separate type of filter instead of being created from SelectorDimFilter, given that the parameters on the filter are being used differently.

The equal columns use case cares about the list of dimensions and ignores the selector value and single dimension, while the original constant value comparison case uses the value/single dimension and ignores the list of dimensions.

I think it'd be cleaner with fewer rules about what needs to be specified if the filters are totally split since the only common point now is the singular extractionFn.

Separation would also help remove the restriction on only having one extraction function for the equal columns case.

[gianm]

@jon-wei, I think you're right that this bears little resemblance to the rest of "selector" and makes more sense as a different DimFilter class. I agree with that.

On using DimensionSpecs instead of dimension names + an extractionFn, I could take or leave that one, so I would be ok with this patch either way. I'm ok with leaving that until someone actually has the need for it.

[jihoonson]

+1 for making a separate class.

[erikdubbelboer]

I should read the rest before I post :)

[erikdubbelboer]

Is there any other filter that allows a DimensionSpec for the dimension?

I don't see how this would work with the extractionFn seeing as this needs to be applied in the ValueMatcher?

[gianm]

IMO, it's not necessary to generalize to having a different extractionFn for each column in this patch, but it does make sense to split this out from the "selector" filter.

[gianm]

Agree with @jon-wei that this has drifted far enough from "selector" that it makes more sense as a separate filter. How about calling it "columnComparison" (& calling the implementation classes ColumnComparisonDimFilter, ColumnComparisonFilter)?

I think "equalColumns" isn't the right name, since what it does on multi-value columns isn't really an "equals" relationship. For one thing it's not transitive, since with the current impl we'd have ["a","b"] == ["a"] and ["a","b"] == ["b"] but ["a"] != ["b"].

Please also document what the filter does on multi-value columns.

[gianm]

@erikdubbelboer, I'm going to target this for 0.10.1, assuming you have time to finish working on it. I think this should be good to go if the filter is split out and has docs added for multi-value behavior.

[jon-wei]

I'm okay with this PR going with the shared extractionFn for now, I'll review again after splitting out the filter

[jihoonson]

nit: redundant null check

[jihoonson]

This looks good, but it's desired to use CacheKeyBuilder. It will be much easier to use and understand.

[jihoonson]

+1 for making a separate class.

[erikdubbelboer]

I think we should decide on the extractionFn. If I add a shared one now but in the future you want to switch to DimensionSpec instead we would have to deprecate the shared one again. Better to decide on a final solution now?

[gianm]

There isn't any other filter that does take DimensionSpec and IIRC, the reason is that DimensionSpecs do things that we don't necessarily need or want in most filters (like decorating). The decorating doesn't work with indexes, and most other filters use indexes, so they would have problems accepting DimensionSpecs.

But this filter doesn't use indexes. So it could use DimensionSpecs and still get its job done.

I don't see how this would work with the extractionFn seeing as this needs to be applied in the ValueMatcher?

If I understand what you're talking about correctly, then DimensionHandlerUtils can handle this. You can just pass in the DimensionSpec and don't worry about it -- the value that comes out of the returned selector will be extractionFn'd appropriately.

[gianm]

Given that this filter wouldn't use indexes I think it makes sense to use DimensionSpecs.

[erikdubbelboer]

I guess it's better if I split these into multiple lines, or how do you guys like this formatted?

[jon-wei]

You can go with:

    ColumnComparisonDimFilter columnComparisonDimFilter = new ColumnComparisonDimFilter(ImmutableList.<DimensionSpec>of(
        DefaultDimensionSpec.of("abc"),
        DefaultDimensionSpec.of("d")
    ));

I just autoformatted that with our style standards:
https://github.com/druid-io/druid/raw/master/druid_intellij_formatting.xml
https://github.com/druid-io/druid/raw/master/eclipse_formatting.xml

[erikdubbelboer]

I tried the eclipse formatting but that results in a completely different style than all other sources. It turns this line into:

ColumnComparisonDimFilter columnComparisonDimFilter = new ColumnComparisonDimFilter(
    ImmutableList.<DimensionSpec> of(DefaultDimensionSpec.of("abc"), DefaultDimensionSpec.of("d")));

Is the eclipse config up to date or is everyone using intellij?

[erikdubbelboer]

I changed it to a new filter now and added DimensionSpecs as inputs. Any input on what should change or maybe which other tests should exist?

[jon-wei]

Looks good generally, had a few comments on formatting

For additional tests, can you add tests that use the ColumnComparisonFilter on long and float columns?

You can look at LongFilteringTest and FloatFilteringTest for reference or add cases there

[jon-wei]

EqualColumnsFilter -> ColumnComparisonFilter

[jon-wei]

You can go with:

    ColumnComparisonDimFilter columnComparisonDimFilter = new ColumnComparisonDimFilter(ImmutableList.<DimensionSpec>of(
        DefaultDimensionSpec.of("abc"),
        DefaultDimensionSpec.of("d")
    ));

I just autoformatted that with our style standards:
https://github.com/druid-io/druid/raw/master/druid_intellij_formatting.xml
https://github.com/druid-io/druid/raw/master/eclipse_formatting.xml

[jon-wei]

maybe rename this test to something else, since dim2 is a multi-value string column

[jon-wei]

formatting, new lines would be easier to read

[erikdubbelboer]

I made all changes. I didn't really make a new test for long and float but instead added a long and float value to the current tests.

[gianm]

Thanks @erikdubbelboer, will take another look today.

[gianm]

LGTM subject to the cache key id being changed. thanks @erikdubbelboer!

[gianm]

I think the trailing ### is ignored, so this is probably harmless, but it's also not necessary.

[gianm]

This filter should have a new cache id, like DimFilterUtils.COLUMN_COMPARISON_CACHE_ID.

[erikdubbelboer]

I changed the cache key.

[gianm]

LGTM, thanks!

[erikdubbelboer]

The failed test has nothing to do with this pull request. I get timeouts like that when I test locally as well. Maybe the test timeouts should be increased a bit.

[gianm]

I restarted the tester.

That test has a history of being a little elusive, see some attempts to fix it at: https://github.com/druid-io/druid/search?q=druidcoordinatortest&type=Issues&utf8=%E2%9C%93.

[erikdubbelboer]

For me increasing the 60_000L test timeout to 120_000L seems to work. But I guess that could also just be luck.

[jon-wei]

Looks good, thanks for the contribution!