Hi,

I was recently asked to add some granularity settings.
I wish it were an easy process to do so. Had to patch adapter.inc and query_type_date.inc, but got to a working solution which may benefit the community.

The patch is based on work done in Search API.

Best,
Dan

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zerolab’s picture

and the patch

zerolab’s picture

Status: Active » Needs review
SamChez’s picture

Hey zerolab, thanks for the contribution. I ported it to 7.x-1.x-dev. In the ported version, I changed the default granularity to be day instead of minute, because day might be used a bit for frequently than minute.

Also, since this is for D7, should I be posting this ported patch here or as a new issue with the version tag 7.x-1.x-dev?

Status: Needs review » Needs work

The last submitted patch, apachesolr-date-granularity-1952296-3.patch, failed testing.

jongagne’s picture

Status: Needs work » Patch (to be ported)

My coworker of mine was logged into my computer and so I posted Patch #3 on his account. It failed because it was being tested against the 6.x-3.0-rc1 branch instead of the 7.0-1.x-dev branch. I'm not familiar with standard practices with porting, but I'll create a new issue for the port under the appropriate branch. If anyone is familiar with different standard practices for this process, please advise. Thanks.

jongagne’s picture

Status: Patch (to be ported) » Needs review
David_Rothstein’s picture

Version: 6.x-3.0-rc1 » 7.x-1.x-dev
FileSize
2.33 KB

This mostly seems to work but here's a reroll fixing a few things:

  1. There were PHP notices in the case where a date range hadn't been selected yet.
  2. -        $next_gap = facetapi_get_next_date_gap($gap, FACETAPI_DATE_SECOND);
    +        $next_gap = facetapi_get_next_date_gap($gap, $granularity - 1);
    

    This can't be right since $granularity is a string. I actually just removed this whole section from the patch because it seemed to me that this issue is more about limiting the options that are displayed inside the facet rather than limiting the query.

  3. +        FACETAPI_DATE_SECOND => t('Second'),
    

    I don't think this can be an option in the UI since there's code elsewhere that prevents it? So I removed it.

  4. For backwards compatibility it seemed to me that the default has to be minute (rather than day), even though I agree day makes more sense. So I switched it back to minute.
  5. Other small fixes.

I also tried to simplify the code a bit using facetapi_get_next_date_gap() but then realized there's an edge case where my simplification fails: If, for example, you configure the facet to have granularity of "year" but the actual date range of all your site's data is within a single month, it will not work and you'll get the full drilldown again. So we could go back to the other method, I suppose, though in practice I think it takes effort to get into this situation since it only happens if the actual range of your data is way smaller than you expected it to be, in which case maybe the system knows better than you :)

The other thing I was wondering about this patch is whether the admin form is really defined in the right place in the code? It looked funny but I didn't know a better place offhand.

David_Rothstein’s picture

Actually, there's an even simpler way to write the code, and it avoids the edge case I mentioned above also. We can simply use strtotime() to compare the gap and granularity for us.

zerolab’s picture

Hi jongagne, David_Rothstein,

thank you for the patch tweaks, they look great.

I had the same thought about the admin forms, but could not find any simpler or easier way to do it. Also could not find a more suitable place either.

Cheers,
Dan

Nick_vh’s picture

Version: 7.x-1.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

Changed SECOND, that was a string to the FACETAPI_DATE_SECOND constant and tested the patch. Works fine but has some issues with date facets (which is the problem of date facets).

Committing this and needs backport

kevin.dutra’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
2.44 KB

Here's an equivalent D6 version to the patch in #8.