Default filterHandler is inefficient

[esjewett]

This is the default filterHandler in the base mixin:

var _filterHandler = function (dimension, filters) {
        dimension.filter(null);

        if (filters.length === 0) {
            dimension.filter(null);
        } else {
            dimension.filterFunction(function (d) {
                for (var i = 0; i < filters.length; i++) {
                    var filter = filters[i];
                    if (filter.isFiltered && filter.isFiltered(d)) {
                        return true;
                    } else if (filter <= d && filter >= d) {
                        return true;
                    }
                }
                return false;
            });
        }
        return filters;
    };

Every time a filter is applied to a chart, the current filter on the dimension is removed and then a new one is added. This is extremely inefficient. I believe it renders all of the benefit of Crossfilter's optimization for incremental filter change moot. Instead, the filter should be removed or a new filterFunction should be applied. Depending on the filter type, we could even optimize further and use dimension.rangeFilter or dimension.exactFilter.

Can we start by removing the first line where dimension.filter(null); or is this doing something somewhere that I don't understand?

[gordonwoodhull]

That makes sense. Someone must have thought that filters were cumulative at some point.

Please try it out and run the test suite. Hopefully that will catch any issues. The suite is extensive but never complete.

Related: #478, not using range filters is also shredding crossfilter performance.

[esjewett]

Digging in a little now. Hopefully I can move to using filterExact or filterRange when appropriate as well.

[esjewett]

BTW - which branch should I send a pull request against?

[gordonwoodhull]

Thanks @esjewett, for straight bug fixes it's master/2.0. This doesn't present any interface changes, and I don't think it's very risky because any problems will be easy to detect.

[esjewett]

For some reason the filterRange part of this fails tests, but can't see why. I will submit a pull request with just the removal of the extra null filter and the filterExact part.

if (filters.length === 0) {
            dimension.filter(null);
        } else if (filters.length === 1 && !Array.isArray(filters[0])) {
            dimension.filterExact(filters[0]);
        } else if(filters.length === 1 && Array.isArray(filters[0])
            && filters[0].isFiltered
            && typeof filters[0][0] === typeof filters[0][1]) {

            dimension.filterRange(filters[0]);
        } else {
            dimension.filterFunction(function (d) {
                for (var i = 0; i < filters.length; i++) {
                    var filter = filters[i];
                    if (filter.isFiltered && filter.isFiltered(d)) {
                        return true;
                    } else if (filter <= d && filter >= d) {
                        return true;
                    }
                }
                return false;
            });
        }

[gordonwoodhull]

woof, that is some complicated logic in that case, i am not surprised. thanks for noting it.

[esjewett]

I had also tried adding a isRangedFilter property to the range array returned by dc.filters.RangedFilter (property value was just JS true) and then testing on that. But that never seemed to test as true. I wonder if unexpected properties are stripped somewhere. Will look at it further at some point!

[gordonwoodhull]

Yeah, I'm adding a filterType field to all of the dc.filters - it will also help people who need to serialize the filters. It seems to work! And also improves performance, although not as much as just enabling crossfilter as your patch does!

[esjewett]

Ah, the filterType is great, actually. In my WebWorker thing I do serialize filters. It's not a problem right now because all my filters are either exact values or ranges, but when I make things more general it will become an issue.

[gordonwoodhull]

Fixed in 2.0 beta 19