Date: Tue, 23 Oct 2012 00:52:55 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [PATCH v5 6/9] lib: add date range query support
Message-ID: <20121023045255.GQ14861@mit.edu>
References: <cover.1350854171.git.jani@nikula.org>
	<f21b8702728457c087478b26700e9448bc16c61d.1350854171.git.jani@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <f21b8702728457c087478b26700e9448bc16c61d.1350854171.git.jani@nikula.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Oct 22 at 12:22 am:
> Add a custom value range processor to enable date and time searches of
> the form date:since..until, where "since" and "until" are expressions
> understood by the previously added date/time parser, to restrict the
> results to messages within a particular time range (based on the Date:
> header).
> 
> If "since" or "until" describes date/time at an accuracy of days or
> less, the values are rounded according to the accuracy, towards past
> for "since" and towards future for "until". For example,
> date:november..yesterday would match from the beginning of November
> until the end of yesterday. Expressions such as date:today..today
> means since the beginning of today until the end of today.
> 
> Open-ended ranges are supported (since Xapian 1.2.1), i.e. you can
> specify date:..until or date:since.. to not limit the start or end
> date, respectively.
> 
> CAVEATS:
> 
> Xapian does not support spaces in range expressions. You can replace
> the spaces with '_', or (in most cases) '-', or (in some cases) leave
> the spaces out altogether.
> 
> Entering date:expr without ".." (for example date:yesterday) will not
> work as you might expect. You can achieve the expected result by
> duplicating the expr both sides of ".." (for example
> date:yesterday..yesterday).
> 
> Open-ended ranges won't work with pre-1.2.1 Xapian, but they don't
> produce an error either.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  lib/Makefile.local     |    1 +
>  lib/database-private.h |    1 +
>  lib/database.cc        |    5 +++++
>  lib/parse-time-vrp.cc  |   40 ++++++++++++++++++++++++++++++++++++++++
>  lib/parse-time-vrp.h   |   19 +++++++++++++++++++
>  5 files changed, 66 insertions(+)
>  create mode 100644 lib/parse-time-vrp.cc
>  create mode 100644 lib/parse-time-vrp.h
> 
> diff --git a/lib/Makefile.local b/lib/Makefile.local
> index d1635cf..6c0f42f 100644
> --- a/lib/Makefile.local
> +++ b/lib/Makefile.local
> @@ -58,6 +58,7 @@ libnotmuch_c_srcs =		\
>  
>  libnotmuch_cxx_srcs =		\
>  	$(dir)/database.cc	\
> +	$(dir)/parse-time-vrp.cc	\
>  	$(dir)/directory.cc	\
>  	$(dir)/index.cc		\
>  	$(dir)/message.cc	\
> diff --git a/lib/database-private.h b/lib/database-private.h
> index 88532d5..d3e65fd 100644
> --- a/lib/database-private.h
> +++ b/lib/database-private.h
> @@ -52,6 +52,7 @@ struct _notmuch_database {
>      Xapian::QueryParser *query_parser;
>      Xapian::TermGenerator *term_gen;
>      Xapian::ValueRangeProcessor *value_range_processor;
> +    Xapian::ValueRangeProcessor *date_range_processor;
>  };
>  
>  /* Return the list of terms from the given iterator matching a prefix.
> diff --git a/lib/database.cc b/lib/database.cc
> index 761dc1a..4df3217 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "database-private.h"
> +#include "parse-time-vrp.h"
>  
>  #include <iostream>
>  
> @@ -710,12 +711,14 @@ notmuch_database_open (const char *path,
>  	notmuch->term_gen = new Xapian::TermGenerator;
>  	notmuch->term_gen->set_stemmer (Xapian::Stem ("english"));
>  	notmuch->value_range_processor = new Xapian::NumberValueRangeProcessor (NOTMUCH_VALUE_TIMESTAMP);
> +	notmuch->date_range_processor = new ParseTimeValueRangeProcessor (NOTMUCH_VALUE_TIMESTAMP);
>  
>  	notmuch->query_parser->set_default_op (Xapian::Query::OP_AND);
>  	notmuch->query_parser->set_database (*notmuch->xapian_db);
>  	notmuch->query_parser->set_stemmer (Xapian::Stem ("english"));
>  	notmuch->query_parser->set_stemming_strategy (Xapian::QueryParser::STEM_SOME);
>  	notmuch->query_parser->add_valuerangeprocessor (notmuch->value_range_processor);
> +	notmuch->query_parser->add_valuerangeprocessor (notmuch->date_range_processor);
>  
>  	for (i = 0; i < ARRAY_SIZE (BOOLEAN_PREFIX_EXTERNAL); i++) {
>  	    prefix_t *prefix = &BOOLEAN_PREFIX_EXTERNAL[i];
> @@ -778,6 +781,8 @@ notmuch_database_close (notmuch_database_t *notmuch)
>      notmuch->xapian_db = NULL;
>      delete notmuch->value_range_processor;
>      notmuch->value_range_processor = NULL;
> +    delete notmuch->date_range_processor;
> +    notmuch->date_range_processor = NULL;
>  }
>  
>  void
> diff --git a/lib/parse-time-vrp.cc b/lib/parse-time-vrp.cc
> new file mode 100644
> index 0000000..7e4eca4
> --- /dev/null
> +++ b/lib/parse-time-vrp.cc
> @@ -0,0 +1,40 @@

Should this file have the usual preamble?

> +
> +#include "database-private.h"
> +#include "parse-time-vrp.h"
> +#include "parse-time-string.h"
> +
> +#define PREFIX "date:"
> +
> +/* See *ValueRangeProcessor in xapian-core/api/valuerangeproc.cc */
> +Xapian::valueno
> +ParseTimeValueRangeProcessor::operator() (std::string &begin, std::string &end)
> +{
> +    time_t t, now;
> +
> +    /* Require date: prefix in start of the range... */
> +    if (STRNCMP_LITERAL (begin.c_str (), PREFIX))

Could be
  if (begin.rfind (PREFIX, 0) == string::npos)
but that may not be clearer.

> +	return Xapian::BAD_VALUENO;
> +
> +    /* ...and remove it. */
> +    begin.erase (0, sizeof (PREFIX) - 1);
> +
> +    /* Use the same 'now' for begin and end. */
> +    if (time (&now) == (time_t) -1)
> +	return Xapian::BAD_VALUENO;
> +
> +    if (!begin.empty ()) {
> +	if (parse_time_string (begin.c_str (), &t, &now, PARSE_TIME_ROUND_DOWN))
> +	    return Xapian::BAD_VALUENO;
> +
> +	begin.assign (Xapian::sortable_serialise ((double) t));
> +    }
> +
> +    if (!end.empty ()) {
> +	if (parse_time_string (end.c_str (), &t, &now, PARSE_TIME_ROUND_UP_INCLUSIVE))
> +	    return Xapian::BAD_VALUENO;
> +
> +	end.assign (Xapian::sortable_serialise ((double) t));
> +    }
> +
> +    return valno;
> +}
> diff --git a/lib/parse-time-vrp.h b/lib/parse-time-vrp.h
> new file mode 100644
> index 0000000..526c217
> --- /dev/null
> +++ b/lib/parse-time-vrp.h
> @@ -0,0 +1,19 @@

Same thing about the preamble.

> +
> +#ifndef NOTMUCH_PARSE_TIME_VRP_H
> +#define NOTMUCH_PARSE_TIME_VRP_H
> +
> +#include <xapian.h>
> +
> +/* see *ValueRangeProcessor in xapian-core/include/xapian/queryparser.h */

Out of curiosity, why the Xapian source reference?
ValueRangeProcessor is documented along the rest of Xapian.

> +class ParseTimeValueRangeProcessor : public Xapian::ValueRangeProcessor {
> +protected:
> +    Xapian::valueno valno;
> +
> +public:
> +    ParseTimeValueRangeProcessor (Xapian::valueno slot_)
> +	: valno(slot_) { }
> +
> +    Xapian::valueno operator() (std::string &begin, std::string &end);
> +};
> +
> +#endif /* NOTMUCH_PARSE_TIME_VRP_H */