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.

Sign up for GitHub

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

Jump to bottom

Add support for facet ranges #76

Closed
wants to merge 6 commits into from
Closed

Add support for facet ranges #76

wants to merge 6 commits into from

Conversation

mehaase
Copy link
Contributor

@mehaase mehaase commented Oct 30, 2013

Charles Nagy has a fork that has nice support for facet ranges. I extended his fork to support date ranges in facets.

Because Nagy's fork has diverged so far from yours, I fast-forwarded to the latest version of master in your repository, then cherry picked the relevant commits from his repo and my repo. Therefore, this should merge cleanly!

It includes updated documentation, too. Please let me know if there's anything I should change before you will merge this.

Thanks,

@tow
Copy link
Owner

tow commented Nov 12, 2013

Thanks for this, this is really useful, and looks well written. A few comments:

  1. could you remove the logging call method in sunburnt.py

  2. could you use the schema.py:solr_date object to check & store datetime objects? That will use mx.DateTime if available (which supports a wider range of dates than native Python datetime); it also ensures the codebase is consistent in datetime handling.

  3. Can you add a check that range.start, range.gap, and range.end are
    (i) comparable - eg you've not got a numeric start and a datetime end.
    (ii) sanely positioned - eg that range.end > range.start etc

  4. Is there a reason you've required that the range queries be only int or datetime? my reading of the solr docs suggests that they can be any field on which you can do range queries, ie floats should be ok - in fact even strings should be fine.

  5. not a blocker, but it might be nice to include support for facet.range.hardened, facet.range.other, facet.range.include

  6. finally, could you add tests for the query generation?

@tow tow mentioned this pull request Nov 12, 2013
Closed
@mehaase
Copy link
Contributor Author

mehaase commented Nov 12, 2013

Sure, I'd be happy to, but I'm swamped this week. I will try to fix that stuff up as soon as I can.

Cheers,

@mehaase mehaase closed this Nov 12, 2013
@mehaase mehaase reopened this Nov 12, 2013
Mark E. Haase added 2 commits March 14, 2014 14:40
While working on #72, I noticed some tests were broken.
55ddec0 
There was 2 layers of error: first there were a handful of test errors due
to comparing naive and aware datetimes. I can't tell exactly what the purpose
of these tests was supposed to be (why compare a naive date to an aware date
in the first place?) but I fixed it by modifying the test to make the naive
date an aware date - if necessary - before comparison.

After fixing that, there was another test failure revealed where 34.123Z was
yielding 129999 microseconds instead of 130000 microseconds, due to lack of
floating point precision in dates.py:datetime_factory. I modified this function
to use integer math instead in order to get a precise result.
temp commit - planning to rebase
feec885
@mehaase
Copy link
Contributor Author

mehaase commented Mar 17, 2014

Toby, I have this done, but I ended up squashing commits so I'm going to close this and open a new pull request.

@mehaase mehaase closed this Mar 17, 2014
@mehaase mehaase mentioned this pull request Mar 17, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mehaase @tow