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.

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

Fixes bug 497 - Filtering date dimensions for row/pie #555

Merged
merged 4 commits into from
Jul 7, 2014

Conversation

mtraynham
Copy link
Contributor

Fixes Bug #497.

Correctly coerce dates to primitives for comparison.

…ltering dates. We use a trick from Crossfilter 'a <= b && a >= b' instead of 'a == b' because the former coerces the values to primitives which then works for dates.
… changes in filterHandler. filters may be dates, thus indexOf will not work.
@mtraynham
Copy link
Contributor Author

Added some changes to removeFilter and hasFilter. Dates are objects and indexOf relies on the fact that they are the same objects in memory, but it really needs to be a coercive comparison.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Jun 5, 2014
@gordonwoodhull gordonwoodhull merged commit 18e593e into dc-js:master Jul 7, 2014
gordonwoodhull added a commit that referenced this pull request Jul 7, 2014
@gordonwoodhull
Copy link
Contributor

Hi @mtraynham, I am finally merging this. Thanks!

I figured you probably wouldn't mind me snagging parts of your fiddle for the jasmine spec. If you have a minute, could you double check the test in 9c682c9? It definitely fails before your patch and succeeds after, but it fails differently from your fiddle: whereas your fiddle usually shows little stubs of wrong values, this just gets a straight zero wrong value.

@mtraynham
Copy link
Contributor Author

Interesting. I have to fix that fiddle and try it out.

The stubs left over should be correct. When you filter, you take the rows dimensional value. Since this corresponds to one of the dates in crossfilter index, it should match at most the one date reference. The other dates (that are same the date) would be the ones that failed to filter.

@mtraynham
Copy link
Contributor Author

So broken version:
http://jsfiddle.net/SjT8E/4/

Fixed version:
http://jsfiddle.net/SjT8E/2/

Seems like the number's match up properly when you filter on Charts 3 & 5 now. In the broken version, when you filter, the stub left over should be a subset of what should actually be returned.

@gordonwoodhull
Copy link
Contributor

I linked to updated fiddles in the test. Maybe the difference is that I am filtering directly on a new Date in the test rather than a value from the dataset? Anyway, thanks very much for the test - it makes me uncomfortable to fix a bug without having a test to verify that it stays fixed.

@mtraynham
Copy link
Contributor Author

Ahh yes, didn't think to check how you were filtering. You are correct, because the new Date is not actually a value in the crossfilter index, there would be no reference match at all and thus everything is filtered.

@gordonwoodhull
Copy link
Contributor

Ah, and now I get it - it was previously comparing the object references for equality, and JavaScript's == for object references tests identity not value.

So:

> new Date(2014,6,7)==new Date(2014,6,7)
false

I guess the reason for using two comparisons instead of .valueOf() is that .valueOf() would cause primitives to get "boxed" in temporary objects, which is slower than the extra comparison.

@mtraynham
Copy link
Contributor Author

Correctamundo! Thanks again for accepting the pull request :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants