Ticket #452: sortemptylast.patch

tests/api_sorting.cc

@@ -2 +2 @@
  * @brief tests of MSet sorting
  */
 /* Copyright (C) 2007,2008,2009 Olly Betts
+ * Copyright (C) 2010 Richard Boulton
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -150 +151 @@
     return true;
 }
 
+// Test sort functor with some empty values.
+DEFINE_TESTCASE(sortfunctor3, backend && !remote && valuestats) {
+    Xapian::Database db(get_database("apitest_sortrel"));
+    Xapian::Enquire enquire(db);
+    enquire.set_query(Xapian::Query("woman"));
+
+    // Value 10 is set to 'a' for 1, 3, 4, 5, 8, 9, and not set otherwise.
+    {
+        // Test default sort order - missing values come first.
+        Xapian::MultiValueKeyMaker sorter;
+        sorter.add_value(10);
+
+        enquire.set_sort_by_key(&sorter, false);
+        Xapian::MSet mset = enquire.get_mset(0, 10);
+        mset_expect_order(mset, 2, 6, 7, 1, 3, 4, 5, 8, 9);
+    }
+
+    {
+        // Use a default value to put the missing values to the end.
+        Xapian::MultiValueKeyMaker sorter;
+        sorter.add_value(10, false, db.get_value_upper_bound(10) + '\xff');
+
+        enquire.set_sort_by_key(&sorter, false);
+        Xapian::MSet mset = enquire.get_mset(0, 10);
+        mset_expect_order(mset, 1, 3, 4, 5, 8, 9, 2, 6, 7);
+    }
+
+    {
+        // Test using a default value and sorting in reverse order
+        Xapian::MultiValueKeyMaker sorter;
+        sorter.add_value(10, false, db.get_value_upper_bound(10) + '\xff');
+
+        enquire.set_sort_by_key(&sorter, true);
+        Xapian::MSet mset = enquire.get_mset(0, 10);
+        mset_expect_order(mset, 2, 6, 7, 1, 3, 4, 5, 8, 9);
+    }
+
+    {
+        // Test using a default value and generating reverse order keys
+        Xapian::MultiValueKeyMaker sorter;
+        sorter.add_value(10, true, db.get_value_upper_bound(10) + '\xff');
+
+        enquire.set_sort_by_key(&sorter, false);
+        Xapian::MSet mset = enquire.get_mset(0, 10);
+        mset_expect_order(mset, 2, 6, 7, 1, 3, 4, 5, 8, 9);
+    }
+
+    {
+        // Test using a default value, generating reverse order keys, and
+        // sorting in reverse order
+        Xapian::MultiValueKeyMaker sorter;
+        sorter.add_value(10, true, db.get_value_upper_bound(10) + '\xff');
+
+        enquire.set_sort_by_key(&sorter, true);
+        Xapian::MSet mset = enquire.get_mset(0, 10);
+        mset_expect_order(mset, 1, 3, 4, 5, 8, 9, 2, 6, 7);
+    }
+
+    return true;
+}
+
 class NeverUseMeKeyMaker : public Xapian::KeyMaker {
   public:
     std::string operator() (const Xapian::Document &) const
@@ -218 +280 @@
     doc.add_value(3, "xyz");
     TEST_EQUAL(sorter(doc), string("\0\0f\0\xffo\0\0\0\0xyz", 13));
 
+    // An empty slot at the end, in reverse order, is terminated with \xff\xff
     sorter.add_value(4, true);
     TEST_EQUAL(sorter(doc), string("\0\0f\0\xffo\0\0\0\0xyz\0\0\xff\xff", 17));
 
+    // An empty slot at the end, in ascending order, has no effect
+    sorter.add_value(0);
+    TEST_EQUAL(sorter(doc), string("\0\0f\0\xffo\0\0\0\0xyz\0\0\xff\xff", 17));
+
+    // An empty slot at the end, with a default value
+    sorter.add_value(0, false, "hi");
+    TEST_EQUAL(sorter(doc), string("\0\0f\0\xffo\0\0\0\0xyz\0\0\xff\xff\0\0hi",
+                                   21));
+
+    // An empty slot at the end, with a default value, in reverse sort order
+    sorter.add_value(0, true, "hi");
+    TEST_EQUAL(sorter(doc), string("\0\0f\0\xffo\0\0\0\0xyz\0\0\xff\xff\0\0hi"
+                                   "\0\0\x97\x96\xff\xff", 27));
+
     return true;
 }
 

include/xapian/keymaker.h

@@ -61 +61 @@
  *  Other than this, it isn't useful to set @a reverse for collapsing.
  */
 class XAPIAN_VISIBILITY_DEFAULT MultiValueKeyMaker : public KeyMaker {
-    std::vector<std::pair<Xapian::valueno, bool> > valnos;
+    struct KeySpec {
+        Xapian::valueno valno;
+        bool reverse;
+        std::string defvalue;
+        KeySpec(Xapian::valueno valno_, bool reverse_,
+                const std::string & defvalue_)
+                : valno(valno_), reverse(reverse_), defvalue(defvalue_)
+        {}
+    };
+    std::vector<KeySpec> valnos;
 
   public:
     MultiValueKeyMaker() { }
@@ -73 +82 @@
 
     virtual std::string operator()(const Xapian::Document & doc) const;
 
-    void add_value(Xapian::valueno valno, bool reverse = false) {
-        valnos.push_back(std::make_pair(valno, reverse));
+    void add_value(Xapian::valueno valno, bool reverse = false,
+                   const std::string & defvalue = std::string()) {
+        valnos.push_back(KeySpec(valno, reverse, defvalue));
     }
 };
 

api/keymaker.cc

@@ -38 +38 @@
 {
     string result;
 
-    vector<pair<Xapian::valueno, bool> >::const_iterator i = valnos.begin();
+    vector<KeySpec>::const_iterator i = valnos.begin();
     // Don't crash if valnos is empty.
     if (rare(i == valnos.end())) return result;
 
@@ -48 +48 @@
         // be adjusted.
         //
         // FIXME: allow Xapian::BAD_VALNO to mean "relevance?"
-        string v = doc.get_value(i->first);
-        bool reverse_sort = i->second;
+        string v = doc.get_value(i->valno);
+        bool reverse_sort = i->reverse;
 
+        if (v.empty()) {
+            v = i->defvalue;
+        }
+
         if (reverse_sort || !v.empty())
             last_not_empty_forwards = result.size();