User "Catrope" changed the status of MediaWiki.r87310. Old Status: new New Status: fixme
User "Catrope" also posted a comment on MediaWiki.r87310. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/87310#c16514 Commit summary: Adding sql to create table for article feedback stats relating to high/low article ratings; Adding maintenance script to periodically populate high/low article feedback stats table Comment: <pre> + public function formatTimestamp( $unix_time ) { + if ( !is_numeric( $unix_time )) { + throw new InvalidArgumentException( 'Timestamp must be numeric.' ); + } + return date( 'Ymdhis', $unix_time ); + } </pre> We have this built-in as <code>$dbw->timestamp()</code> . Remember that different DBMSes have different timestamp formats. When converting timestamps for non-DB purposes, there's <code>wfTimestamp()</code> . Also, there is no <code>InvalidArgumentException</code> class that I can find. <pre> + 'afshl_avg_ratings' => json_encode( $data[ 'avg_ratings' ] ), </pre> <code>json_encode()</code> is not guaranteed to be available or non-broken, use <code>FormatJson::encode()</code> <pre> + array( array( 'afshl_page_id', 'afshl_avg_overall', 'afshl_avg_ratings', 'afshl_ts' )), </pre> If this set of fields is supposed to form a unique combination, you have to add a UNIQUE index on it, or this replace call will work in Postgres but not in MySQL. Generally, the table doesn't have any indices other than the primary key. <pre> + array( + 'aa_page_id', + 'avg(aa_rating_value) as avg_rating' + ), + 'aa_timestamp > ' . $this->getTimestamp(), + __METHOD__, + array( + 'GROUP BY' => 'aa_page_id', + 'ORDER BY' => 'avg_rating DESC', + 'LIMIT' => '50', + ) </pre> This query sounds painful. There is no index on aa_timestamp, for one thing. I'm not sure what the index would have to be exactly in this case because the GROUP BY and ORDER BY are different, I'd have to look that up. <pre> + 'aa_timestamp > ' . $this->getTimestamp() . ' && aa_page_id IN (' . $page_id . ')', </pre> <code>&&</code> instead of <code>AND</code> in SQL? Does that even work? Also, you don't have to build your own <code>IN(...)</code> list, just use <code>array( "aa_timestamp >= $blah", 'aa_page_id' => $arrayOfPageIDs )</code> _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
