Ticket 2141: bugs with weighted popularity lists across year boundary.

Consolidated several separate implementations of the same weighting algorithm into common_sql_weight() and fixed some bugs...
For MySQL, now using timestampdiff() instead of subtraction for the comparison, so we get sane results when the year doesn't match, and utc_timestamp() rather than now() so we don't get negative ages for recent items with local server timezone.
Unknown whether the same problems affect PostgreSQL, but note that it lacks the timestampdiff() SQL function.

diff --git a/actions/favorited.php b/actions/favorited.php
index 150b67b0b09451f752d226486031de07852e32b7..9ffa5b84454093fde32b98460751b60bbab32745 100644 (file)
--- a/actions/favorited.php
+++ b/actions/favorited.php
@@ -185,11 +185,7 @@ class FavoritedAction extends Action
 
     function showContent()
     {
-        if (common_config('db', 'type') == 'pgsql') {
-            $weightexpr='sum(exp(-extract(epoch from (now() - fave.modified)) / %s))';
-        } else {
-            $weightexpr='sum(exp(-(now() - fave.modified) / %s))';
-        }
+        $weightexpr = common_sql_weight('fave.modified', common_config('popular', 'dropoff'));
 
         $qry = 'SELECT notice.*, '.
           $weightexpr . ' as weight ' .
@@ -207,7 +203,7 @@ class FavoritedAction extends Action
         }
 
         $notice = Memcached_DataObject::cachedQuery('Notice',
-                                                    sprintf($qry, common_config('popular', 'dropoff')),
+                                                    $qry,
                                                     600);
 
         $nl = new NoticeList($notice, $this);

diff --git a/actions/publictagcloud.php b/actions/publictagcloud.php
index 5c707402905df7dcd0f9665950384849440992b0..b5b474f132184eebb2c66935537e6d122820ab45 100644 (file)
--- a/actions/publictagcloud.php
+++ b/actions/publictagcloud.php
@@ -105,12 +105,8 @@ class PublictagcloudAction extends Action
 
         #Add the aggregated columns...
         $tags->selectAdd('max(notice_id) as last_notice_id');
-        if(common_config('db','type')=='pgsql') {
-            $calc='sum(exp(-extract(epoch from (now()-created))/%s)) as weight';
-        } else {
-            $calc='sum(exp(-(now() - created)/%s)) as weight';
-        }
-        $tags->selectAdd(sprintf($calc, common_config('tag', 'dropoff')));
+        $calc = common_sql_weight('created', common_config('tag', 'dropoff'));
+        $tags->selectAdd($calc . ' as weight');
         $tags->groupBy('tag');
         $tags->orderBy('weight DESC');
 
@@ -136,10 +132,11 @@ class PublictagcloudAction extends Action
             $this->elementStart('dd');
             $this->elementStart('ul', 'tags xoxo tag-cloud');
             foreach ($tw as $tag => $weight) {
+                common_log(LOG_DEBUG, "$weight/$sum");
                 if ($sum) {
                     $weightedSum = $weight/$sum;
                 } else {
-                    $weightedSum = 1;
+                    $weightedSum = 0.5;
                 }
                 $this->showTag($tag, $weight, $weightedSum);
             }

diff --git a/lib/grouptagcloudsection.php b/lib/grouptagcloudsection.php
index 091cf48457ce0bdb9330d62d78b9ff7363ef1944..14ceda0850b09b6918c4da00517370f6b31bad42 100644 (file)
--- a/lib/grouptagcloudsection.php
+++ b/lib/grouptagcloudsection.php
@@ -58,11 +58,7 @@ class GroupTagCloudSection extends TagCloudSection
 
     function getTags()
     {
-        if (common_config('db', 'type') == 'pgsql') {
-            $weightexpr='sum(exp(-extract(epoch from (now() - notice_tag.created)) / %s))';
-        } else {
-            $weightexpr='sum(exp(-(now() - notice_tag.created) / %s))';
-        }
+        $weightexpr = common_sql_weight('notice_tag.created', common_config('tag', 'dropoff'));
 
         $names = $this->group->getAliases();
 
@@ -99,7 +95,6 @@ class GroupTagCloudSection extends TagCloudSection
 
         $tag = Memcached_DataObject::cachedQuery('Notice_tag',
                                                  sprintf($qry,
-                                                         common_config('tag', 'dropoff'),
                                                          $this->group->id,
                                                          $namestring),
                                                  3600);

diff --git a/lib/personaltagcloudsection.php b/lib/personaltagcloudsection.php
index 0b29d58ca60386b9331de6ad7a330cab9191ca14..091425f926d296f8959322e63c2a3ac770bae41d 100644 (file)
--- a/lib/personaltagcloudsection.php
+++ b/lib/personaltagcloudsection.php
@@ -58,13 +58,9 @@ class PersonalTagCloudSection extends TagCloudSection
 
     function getTags()
     {
-        if (common_config('db', 'type') == 'pgsql') {
-            $weightexpr='sum(exp(-extract(epoch from (now() - notice_tag.created)) / %s))';
-        } else {
-            $weightexpr='sum(exp(-(now() - notice_tag.created) / %s))';
-        }
- 
-       $qry = 'SELECT notice_tag.tag, '.
+        $weightexpr = common_sql_weight('notice_tag.created', common_config('tag', 'dropoff'));
+
+        $qry = 'SELECT notice_tag.tag, '.
           $weightexpr . ' as weight ' .
           'FROM notice_tag JOIN notice ' .
           'ON notice_tag.notice_id = notice.id ' .
@@ -83,7 +79,6 @@ class PersonalTagCloudSection extends TagCloudSection
 
         $tag = Memcached_DataObject::cachedQuery('Notice_tag',
                                                  sprintf($qry,
-                                                         common_config('tag', 'dropoff'),
                                                          $this->user->id),
                                                  3600);
         return $tag;

diff --git a/lib/popularnoticesection.php b/lib/popularnoticesection.php
index 9fbc9d2ddd0c779079ba3b75a154cd44fac12846..fbf9a60ab8112395156ffa26b9a5021293f816b6 100644 (file)
--- a/lib/popularnoticesection.php
+++ b/lib/popularnoticesection.php
@@ -48,17 +48,17 @@ class PopularNoticeSection extends NoticeSection
 {
     function getNotices()
     {
+        // @fixme there should be a common func for this
         if (common_config('db', 'type') == 'pgsql') {
-            $weightexpr='sum(exp(-extract(epoch from (now() - fave.modified)) / %s))';
             if (!empty($this->out->tag)) {
                 $tag = pg_escape_string($this->out->tag);
             }
         } else {
-            $weightexpr='sum(exp(-(now() - fave.modified) / %s))';
             if (!empty($this->out->tag)) {
                  $tag = mysql_escape_string($this->out->tag);
             }
         }
+        $weightexpr = common_sql_weight('fave.modified', common_config('popular', 'dropoff'));
         $qry = "SELECT notice.*, $weightexpr as weight ";
         if(isset($tag)) {
             $qry .= 'FROM notice_tag, notice JOIN fave ON notice.id = fave.notice_id ' .
@@ -78,7 +78,7 @@ class PopularNoticeSection extends NoticeSection
         $qry .= ' LIMIT ' . $limit . ' OFFSET ' . $offset;
 
         $notice = Memcached_DataObject::cachedQuery('Notice',
-                                                    sprintf($qry, common_config('popular', 'dropoff')),
+                                                    $qry,
                                                     1200);
         return $notice;
     }

diff --git a/lib/util.php b/lib/util.php
index 63656b6042b9c8aa957b4c3b17fdbcddf9e10587..50bd0e2ac938de026a3d4b5187823773136a62d0 100644 (file)
--- a/lib/util.php
+++ b/lib/util.php
@@ -908,6 +908,26 @@ function common_sql_date($datetime)
     return strftime('%Y-%m-%d %H:%M:%S', $datetime);
 }
 
+/**
+ * Return an SQL fragment to calculate an age-based weight from a given
+ * timestamp or datetime column.
+ *
+ * @param string $column name of field we're comparing against current time
+ * @param integer $dropoff divisor for age in seconds before exponentiation
+ * @return string SQL fragment
+ */
+function common_sql_weight($column, $dropoff)
+{
+    if (common_config('db', 'type') == 'pgsql') {
+        // PostgreSQL doesn't support timestampdiff function.
+        // @fixme will this use the right time zone?
+        // @fixme does this handle cross-year subtraction correctly?
+        return "sum(exp(-extract(epoch from (now() - $column)) / $dropoff))";
+    } else {
+        return "sum(exp(timestampdiff(second, utc_timestamp(), $column) / $dropoff))";
+    }
+}
+
 function common_redirect($url, $code=307)
 {
     static $status = array(301 => "Moved Permanently",